Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2, 2.4
    • Fix Version/s: 2.8
    • Component/s: SCORM
    • Labels:
    • Testing Instructions:
      Hide

      1. Turn on API debugging mode in scorm
      2. Add the scorm package attached to this issue
      3. Make sure auto commit is turned on in scorm package settings
      4. Open the package and navigate to a SCO.
      5. Stop navigation when you see "LMSSetValue" as the last function call in the debugger.
      6. Wait for 60 seconds, "LMSCommit" should be called automatically.

      Show
      1. Turn on API debugging mode in scorm 2. Add the scorm package attached to this issue 3. Make sure auto commit is turned on in scorm package settings 4. Open the package and navigate to a SCO. 5. Stop navigation when you see "LMSSetValue" as the last function call in the debugger. 6. Wait for 60 seconds, "LMSCommit" should be called automatically.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_28_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      This patch helps ensure that data sent from a SCORM package, via SetValue(), is eventually stored in the database so that student progress data isn't lost. It will be Commit()ed within 60 seconds, in case the network connection is lost, the browser crashes, etc. before the SCORM object explicitly commit()s it.

      Some SCORM packages don't explicitly call Commit() regularly for whatever reason. This patch will Commit() the data 60 seconds afer the values are set, unless the package Commit()s sooner.

      It calls commit only if the client (SCO) hasn't already done so explicitly, and calls it only once per 60 seconds even if many values are pending, and doesn't call it at all if no values are pending.

      I am attaching a patch against 2.4 for Master and for the convenience of those who wish to backport it, a patch based against 2.3.2 is also attached.

      See also:
      https://moodle.org/mod/forum/discuss.php?d=219054

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              raymor Ray Morris added a comment -

              Patch backported to 2.3.2.

              Show
              raymor Ray Morris added a comment - Patch backported to 2.3.2.
              Hide
              raymor Ray Morris added a comment -

              Corrected patch against 2.4.

              Show
              raymor Ray Morris added a comment - Corrected patch against 2.4.
              Hide
              danmarsden Dan Marsden added a comment -

              thanks - I'll try to take a look in a couple of weeks when I've got through the urgent stuff in my inbox! - ping me again if I haven't been back by Feb!

              Show
              danmarsden Dan Marsden added a comment - thanks - I'll try to take a look in a couple of weeks when I've got through the urgent stuff in my inbox! - ping me again if I haven't been back by Feb!
              Hide
              nobelium vignesh p added a comment -

              @Ray Morris - Thanks for your patch.
              @Dan - I'm interested in working on this. Is it ok if I could create a patch for the current master branch.

              Show
              nobelium vignesh p added a comment - @Ray Morris - Thanks for your patch. @Dan - I'm interested in working on this. Is it ok if I could create a patch for the current master branch.
              Hide
              danmarsden Dan Marsden added a comment -

              Vignesh - go for it! - although it will need to sit here until 2.7 has branched and we can commit it for 2.8

              Show
              danmarsden Dan Marsden added a comment - Vignesh - go for it! - although it will need to sit here until 2.7 has branched and we can commit it for 2.8
              Hide
              nobelium vignesh p added a comment - - edited

              @Dan - Which one would you suggest? Turn on - turn off autosave option or a field to set the frequency at which the autosave is done (Say I set it to '120s', the progress will be saved every '120s'. A value of '0s' will turn off the autosave feature).

              Show
              nobelium vignesh p added a comment - - edited @Dan - Which one would you suggest? Turn on - turn off autosave option or a field to set the frequency at which the autosave is done (Say I set it to '120s', the progress will be saved every '120s'. A value of '0s' will turn off the autosave feature).
              Hide
              raymor Ray Morris added a comment -

              Thanks, Vignesh. If the GUI had a setting for frequency, would it be most clear to have it default to a reasonable value, such as 60 or 120? If so, that forecloses the option of having it default to zero, which may be fine as long you think that improved functionality should be the default, as opposed to those who think most things should generally default to the old level of functionality.

              Show
              raymor Ray Morris added a comment - Thanks, Vignesh. If the GUI had a setting for frequency, would it be most clear to have it default to a reasonable value, such as 60 or 120? If so, that forecloses the option of having it default to zero, which may be fine as long you think that improved functionality should be the default, as opposed to those who think most things should generally default to the old level of functionality.
              Hide
              danmarsden Dan Marsden added a comment -

              if we have a setting for frequency that should probably be a site-wide setting rather than per-scorm - the gui should be simple for teachers/users and just allow disable/enable like the current patch - (the setting should sit inside the "compatibility settings" area of the scorm settings.)

              Show
              danmarsden Dan Marsden added a comment - if we have a setting for frequency that should probably be a site-wide setting rather than per-scorm - the gui should be simple for teachers/users and just allow disable/enable like the current patch - (the setting should sit inside the "compatibility settings" area of the scorm settings.)
              Hide
              nobelium vignesh p added a comment -

              Ray Morris & Dan - Thanks for your suggestions. I'm going with Dan's suggestion. Its probably best if we could have just an enable/disable switch. Its more user friendly. We can go for a system wide frequency setting if the need arises.

              Show
              nobelium vignesh p added a comment - Ray Morris & Dan - Thanks for your suggestions. I'm going with Dan's suggestion. Its probably best if we could have just an enable/disable switch. Its more user friendly. We can go for a system wide frequency setting if the need arises.
              Hide
              nobelium vignesh p added a comment - - edited

              Hi Dan, I have created a patch. https://github.com/nobelium/moodle/tree/MDL-37401.
              new setting name is 'autosave' and it has been ported to all the scorm versions(scorm1.2, scorm1.3, aicc)

              Show
              nobelium vignesh p added a comment - - edited Hi Dan, I have created a patch. https://github.com/nobelium/moodle/tree/MDL-37401 . new setting name is 'autosave' and it has been ported to all the scorm versions(scorm1.2, scorm1.3, aicc)
              Hide
              danmarsden Dan Marsden added a comment -

              thanks - we can't submit this for integration until after 2.7 release + on-sync process as 2.7 is now in freeze - by my calculations that makes it around 1st June... - ping me again towards the end of May if I haven't updated the status here.

              Show
              danmarsden Dan Marsden added a comment - thanks - we can't submit this for integration until after 2.7 release + on-sync process as 2.7 is now in freeze - by my calculations that makes it around 1st June... - ping me again towards the end of May if I haven't updated the status here.
              Hide
              danmarsden Dan Marsden added a comment -

              Hi Vignesh - after the JS cache patch lands could you please rebase your patch on master and change the "autosave" config name to "autocommit"

              thanks!

              Show
              danmarsden Dan Marsden added a comment - Hi Vignesh - after the JS cache patch lands could you please rebase your patch on master and change the "autosave" config name to "autocommit" thanks!
              Hide
              nobelium vignesh p added a comment -

              Cool, I'll change it when the JS cache patch lands in master.

              Show
              nobelium vignesh p added a comment - Cool, I'll change it when the JS cache patch lands in master.
              Hide
              raymor Ray Morris added a comment - - edited

              It looks like datamodels/scorm_*.js.php have been renamed to datamodels/scorm_*.js. That means they are no longer parsed as PHP, and therefore cannot call get_config(). Are therre any thoughts on how best to proceed? On my local installation, I now just have it auto-commit after 60 seconds, with no configuration.

              Show
              raymor Ray Morris added a comment - - edited It looks like datamodels/scorm_*.js.php have been renamed to datamodels/scorm_*.js. That means they are no longer parsed as PHP, and therefore cannot call get_config(). Are therre any thoughts on how best to proceed? On my local installation, I now just have it auto-commit after 60 seconds, with no configuration.
              Hide
              nobelium vignesh p added a comment -

              Hi Ray Morris, All the php variables that were accessed in scorn_*.js.php, have to be passed via YUI init call (scorm_api) just like scorn->auto is being passed. I'll create a new patch by the end of this day. https://github.com/moodle/moodle/blob/master/mod/scorm/datamodels/scorm_12.js#L641

              Show
              nobelium vignesh p added a comment - Hi Ray Morris, All the php variables that were accessed in scorn_*.js.php, have to be passed via YUI init call (scorm_api) just like scorn->auto is being passed. I'll create a new patch by the end of this day. https://github.com/moodle/moodle/blob/master/mod/scorm/datamodels/scorm_12.js#L641
              Hide
              nobelium vignesh p added a comment -

              Dan- I have renamed the config name to "autocommit"

              Ray - I have made the change on my branch. Please pull and test it out.

              Note to Integrators: This patch is based on MDL-46397, please merge MDL-46397 before this patch.

              Show
              nobelium vignesh p added a comment - Dan- I have renamed the config name to "autocommit" Ray - I have made the change on my branch. Please pull and test it out. Note to Integrators: This patch is based on MDL-46397 , please merge MDL-46397 before this patch.
              Hide
              danmarsden Dan Marsden added a comment -

              Thanks Vignesh - small issue there - it's checking site config but not module level config.

              instead of checking get_config('scorm', 'autocommit') directly you should check both site level and module level configs

              thanks!

              Show
              danmarsden Dan Marsden added a comment - Thanks Vignesh - small issue there - it's checking site config but not module level config. instead of checking get_config('scorm', 'autocommit') directly you should check both site level and module level configs thanks!
              Hide
              nobelium vignesh p added a comment -

              Hi Dan, I'm a bit lost here. Isn't get_config($plugin_name, $config_name) a module level config? Is there some other way of setting/getting module level config? http://docs.moodle.org/dev/Developer_FAQ#How_do_I_get.2Fset_configuration_settings.3F

              Show
              nobelium vignesh p added a comment - Hi Dan, I'm a bit lost here. Isn't get_config($plugin_name, $config_name) a module level config? Is there some other way of setting/getting module level config? http://docs.moodle.org/dev/Developer_FAQ#How_do_I_get.2Fset_configuration_settings.3F
              Hide
              nobelium vignesh p added a comment -

              Oops, sorry, you meant the scorm package config(unique for all scorm packages)? I just added it. Please check it out. For now I'm ORing the values, so if the global setting is turned on it would autocommit even if module level config is turned off and if global level config is turned off it will autocommit if the module level config is turned on.

              Show
              nobelium vignesh p added a comment - Oops, sorry, you meant the scorm package config(unique for all scorm packages)? I just added it. Please check it out. For now I'm ORing the values, so if the global setting is turned on it would autocommit even if module level config is turned off and if global level config is turned off it will autocommit if the module level config is turned on.
              Hide
              danmarsden Dan Marsden added a comment -

              yeah - sorry I forgot to respond there... and I wasn't very clear!

              you had added a setting to mod_form in the patch but weren't checking it anywhere.... you are checking it now but it won't work as it will also need a new column in the scorm table

              Also the site level config which you are checking using get_config is really just the default value that should be used in mod_form - you only need to check $sorm->autocommit when checking to see if it should be used.

              does that make more sense?

              thanks!

              Show
              danmarsden Dan Marsden added a comment - yeah - sorry I forgot to respond there... and I wasn't very clear! you had added a setting to mod_form in the patch but weren't checking it anywhere.... you are checking it now but it won't work as it will also need a new column in the scorm table Also the site level config which you are checking using get_config is really just the default value that should be used in mod_form - you only need to check $sorm->autocommit when checking to see if it should be used. does that make more sense? thanks!
              Hide
              nobelium vignesh p added a comment -

              Yup, got it. I wanted to ask about adding a column (I have never used the upgrade scripts before). I have used the XMLDB editor to create a new column and added the php code generated to upgrade.php and bumped up the version and copied the required version number from /version.php. Am I doing it right? I have pushed it to https://github.com/nobelium/moodle/tree/MDL-37401 Please take a look

              Show
              nobelium vignesh p added a comment - Yup, got it. I wanted to ask about adding a column (I have never used the upgrade scripts before). I have used the XMLDB editor to create a new column and added the php code generated to upgrade.php and bumped up the version and copied the required version number from /version.php. Am I doing it right? I have pushed it to https://github.com/nobelium/moodle/tree/MDL-37401 Please take a look
              Hide
              danmarsden Dan Marsden added a comment -

              cool - db scripts look great to me! - we just need to add some testing instructions for this as well. I haven't had a chance to review the code completely and test it yet so throwing it up into peer review mode so I remember to look at it again.

              Show
              danmarsden Dan Marsden added a comment - cool - db scripts look great to me! - we just need to add some testing instructions for this as well. I haven't had a chance to review the code completely and test it yet so throwing it up into peer review mode so I remember to look at it again.
              Hide
              cibot CiBoT added a comment -

              Fails against automated checks.

              Checked MDL-37401 using repository: git://github.com/nobelium/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-37401 using repository: git://github.com/nobelium/moodle.git master (branch: MDL-37401 | CI Job ) Coding style problems found Testing instructions are missing. More information about this report
              Hide
              nobelium vignesh p added a comment -

              Hi Dan, I have made some changes to the code (passed an empty string as argument to LMSCommit) and fixed the coding style problem. I have also added some testing instructions, please see if they will be sufficient.

              Show
              nobelium vignesh p added a comment - Hi Dan, I have made some changes to the code (passed an empty string as argument to LMSCommit) and fixed the coding style problem. I have also added some testing instructions, please see if they will be sufficient.
              Hide
              danmarsden Dan Marsden added a comment -

              just had a quick chat with Andrew Nicols as I wasn't sure about a few things....
              https://github.com/nobelium/moodle/commit/f3139d6e2d7a72b89407b2813fa09df4234db78d#diff-205c67a47d35a63e2cadb858a80e634eR747

              == should be using ===

              few spacing issues in line 747 too - shouldn't be spacing in-between brackets "( (" should be "(("

              the function inside line 747 should be broken up onto multiple lines instead of defined on one line.

              window.SetTimeout should be using YUI (Y.later)

              delete SCORMapi1_2.timeout
              should set to null instead of delete:
              SCORMapi1_2.timeout = null;

              thanks Vignesh!

              Dan

              Show
              danmarsden Dan Marsden added a comment - just had a quick chat with Andrew Nicols as I wasn't sure about a few things.... https://github.com/nobelium/moodle/commit/f3139d6e2d7a72b89407b2813fa09df4234db78d#diff-205c67a47d35a63e2cadb858a80e634eR747 == should be using === few spacing issues in line 747 too - shouldn't be spacing in-between brackets "( (" should be "((" the function inside line 747 should be broken up onto multiple lines instead of defined on one line. window.SetTimeout should be using YUI (Y.later) delete SCORMapi1_2.timeout should set to null instead of delete: SCORMapi1_2.timeout = null; thanks Vignesh! Dan
              Hide
              nobelium vignesh p added a comment -

              Hi Dan, Fixed the issue, pushed it just now.

              Show
              nobelium vignesh p added a comment - Hi Dan, Fixed the issue, pushed it just now.
              Hide
              danmarsden Dan Marsden added a comment -

              yeah that looks better - thanks!

              Show
              danmarsden Dan Marsden added a comment - yeah that looks better - thanks!
              Hide
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              marina Marina Glancy added a comment -

              Hi, thanks for working on it.
              I'm afraid it does not work as expected atm.
              After 60 seconds nothing appears in scorm debugging interface but a JS error appears in console TypeError: method is undefined. The backtrace leads to YUI function later().

              Show
              marina Marina Glancy added a comment - Hi, thanks for working on it. I'm afraid it does not work as expected atm. After 60 seconds nothing appears in scorm debugging interface but a JS error appears in console TypeError: method is undefined . The backtrace leads to YUI function later().
              Hide
              danmarsden Dan Marsden added a comment -

              whoops - thanks Marina - Vignesh was using window.SetTimeout and changed to Y.later() but I didn't test it after that change. Vignesh can you please take another look? - thanks!

              Show
              danmarsden Dan Marsden added a comment - whoops - thanks Marina - Vignesh was using window.SetTimeout and changed to Y.later() but I didn't test it after that change. Vignesh can you please take another look? - thanks!
              Hide
              marina Marina Glancy added a comment -

              Thanks for reply Dan, Vignesh please resubmit the issue when ready. Thanks

              Show
              marina Marina Glancy added a comment - Thanks for reply Dan, Vignesh please resubmit the issue when ready. Thanks
              Hide
              nobelium vignesh p added a comment -

              Oops, Sorry I was away. I'll take a look in some time.

              Show
              nobelium vignesh p added a comment - Oops, Sorry I was away. I'll take a look in some time.
              Hide
              cibot CiBoT added a comment -

              Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
              Hide
              cibot CiBoT added a comment -

              +1 code verified against automated checks.

              Checked MDL-37401 using repository: git://github.com/nobelium/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - +1 code verified against automated checks. Checked MDL-37401 using repository: git://github.com/nobelium/moodle.git master (branch: MDL-37401 | CI Job ) More information about this report
              Hide
              nobelium vignesh p added a comment -

              Hi Marina, sorry for last week's untested patch. I have fixed the bug in the patch. Please take a look.

              Show
              nobelium vignesh p added a comment - Hi Marina, sorry for last week's untested patch. I have fixed the bug in the patch. Please take a look.
              Hide
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              poltawski Dan Poltawski added a comment -

              Integrated to master - thanks Vignesh.

              Show
              poltawski Dan Poltawski added a comment - Integrated to master - thanks Vignesh.
              Hide
              ankit_frenz Ankit Agarwal added a comment -

              LMSCommit was called automatically as described.

              Show
              ankit_frenz Ankit Agarwal added a comment - LMSCommit was called automatically as described.
              Hide
              marina Marina Glancy added a comment -

              Thank you, your change is now upstream!

              Show
              marina Marina Glancy added a comment - Thank you, your change is now upstream!

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    10/Nov/14