handleSessionTime #37

canvasplay · 2017-10-17T08:01:41Z

Hi!

I've been using this for years, since some LMS platforms does not handle automatically the session time I have added an option for the wrapper to handle it. Maybe someone would find it helpful too.

If you dont mind, could you pull it? I have followed the same approach as other options like "handleCompletionStatus", adapt it as you consider.

Keep in touch!
Thanks.

…initialize call

moloko · 2017-10-17T16:00:17Z

Seems like a good addition to the wrapper code to me... at the moment anyone wanting to use the wrapper has to provide their own implementation (here's how we do it in the Adapt framework) so having this provided by the wrapper would save anyone wanting to use it from having to create their own implementation - whilst still allowing someone to do so if they choose/need to.

moloko · 2017-10-18T09:32:27Z

On the README for this repo it does say:

These wrappers are intended to make your life easier so you don't need to be a SCORM expert to add SCORM support to your e-learning course

I would argue that having the wrapper handle session time for you goes a long way towards achieving that aim

pipwerks · 2017-10-21T23:42:01Z

Thanks for submitting. I am willing to merge, but I need to be sure it doesn't introduce any bugs. I will take a look when I have some spare time (I've been pretty busy lately, so it my be a while).

In the meantime, if anyone has tested and can confirm it works for them, feedback would be appreciated.

moloko · 2017-10-22T10:28:54Z

i’ll try and have a look over the next week

canvasplay · 2017-11-24T09:52:44Z

Hi guys! Did you have the time to check this?
Can I help somehow? From my side everything is working properly ;)

moloko · 2017-12-21T10:56:15Z

src/JavaScript/SCORM_API_wrapper.js

+                        this.set("cmi.core.session_time",pipwerks.UTILS.MillisecondsToCMIDuration(n)); 
+                    break;
+                    case "2004":
+                        //success = this.set("cmi.session_time",centisecsToISODuration(Math.floor(n/10)));


please remove commented-out line of code

moloko · 2017-12-21T10:56:20Z

src/JavaScript/SCORM_API_wrapper.js

+                    //the time format is different on scorm 1.2 or 2004, so we use 
+                    //different conversions depending on the case 
+                    case "1.2" :
+                        //success = this.set("cmi.core.session_time",MillisecondsToCMIDuration(n)); 


please remove commented-out line of code

moloko · 2017-12-21T10:57:13Z

@canvasplay no sorry it's been a horribly busy few months!

One thing that might help is if you had some unit tests showing that those functions have the correct output for various inputs...

canvasplay · 2017-12-22T10:37:44Z

@moloko, about the unit testing, I'm not a unit testing expert and I don't really know how to fit them in this project... but ok, I accept the challenge :)

I have added in my repo an inital approach to unit testing for the sake of this matter.
I usually test my code using nodejs Assert module so I tried to do it so, but src/JavaScript/SCORM_API_wrapper.js is not an AMD or CommonJs module so it obviously does not work.
There is a pull request for this but looks that is not going to happen soon.

Please could you take a look at my approach and provide some feedback to implement the testing part?
Thanks!

PS: Maybe I am introducing too much complexity to the repo... :D

canvasplay · 2018-01-02T15:28:06Z

Hey! Happy new year! 😄

I have moved forward with QUnit as unit testing engine. I think it is a better way so we can do crossbrowser testing and there is no need of an AMD compatible module.

I have created some basic tests... Did I miss any test case?
Actually I had to fix the functions to pass the tests, it proves they are useful 👍

Looking forward for your feedback.
Cheers!

moloko · 2018-01-11T12:09:44Z

src/JavaScript/SCORM_API_wrapper.js

+
+                var dtm = new Date(); 
+
+                //in the next line you substract the time recorded when initialising 


typo: 'substract' should be 'subtract'

moloko

thanks for doing this! one last comment is that I would add a few more tests for different cs/ms values for each of the two functions you're testing, currently you've only got one test of an actual time in each.

moloko · 2018-01-11T12:11:34Z

src/JavaScript/SCORM_API_wrapper.js

+        str += "T"; 
+        if (nH > 0) str += nH + "H"; 
+        if (nMin > 0) str += nMin + "M"; 
+                if (nCs > 0) str += (nCs / 100) + "S"; 


indentation is a bit off here

moloko · 2018-01-11T12:21:02Z

src/JavaScript/SCORM_API_wrapper.js

+pipwerks.UTILS.MillisecondsToCMIDuration = function(n){
+
+    //Convert duration from milliseconds to 0000:00:00.00 format 
+    n = (!n || n<0)? 0 : n; //default value and force positive duration


could you add some spacing around the operators for consistency with the rest of the code?

moloko · 2018-01-11T12:23:12Z

src/JavaScript/SCORM_API_wrapper.js

+   Return:     String
+---------------------------------------------------------------------------- */
+
+pipwerks.UTILS.MillisecondsToCMIDuration = function(n){


it's just my opinion but I think the function name could be shortened to msToCMIDuration and the intention of it would still be clear. similarly csToISODuration still seems like a clear name to me.

totally agree ;)

moloko · 2018-01-11T12:23:39Z

src/JavaScript/SCORM_API_wrapper.js

+    var h = "0" + Math.floor(n / 3600000); 
+    var m = "0" + dtm.getMinutes(); 
+    var s = "0" + dtm.getSeconds(); 
+    hms = h.substr(h.length-2)+":"+m.substr(m.length-2)+":"; 


could you add some spacing around the operators for consistency (and readability)

moloko · 2018-01-11T12:30:52Z

src/JavaScript/SCORM_API_wrapper.js

+    var nMin = Math.floor(nCs /6000); 
+    nCs -= nMin * 6000 
+    // Now we can construct string 
+    if (nY > 0) str += nY + "Y"; 


I love that you've assumed someone may have spent years in the same session - that would be one dedicated learner ;-) I didn't even bother going above days in my implementation!!

We are not gonna set the limits of the eLearning ;)

moloko · 2018-01-11T12:32:53Z

src/JavaScript/SCORM_API_wrapper.js

+
+pipwerks.UTILS.MillisecondsToCMIDuration = function(n){
+
+    //Convert duration from milliseconds to 0000:00:00.00 format 


could you move this comment to the main comment block above?

moloko · 2018-01-11T13:29:38Z

test/JavaScript/test.js

+    var t2 = 1513938095752;
+    var d = t2 - t1;
+
+    assert.equal(pipwerks.UTILS.centisecsToISODuration(d), 'PT17M11.2S');


could you add descriptions for your tests? e.g.

assert.equal(pipwerks.UTILS.centisecsToISODuration(null), 'PT0H0M0S', 'Check null is correctly handled');

canvasplay · 2018-01-12T11:09:31Z

Besides the formatting changes, the tests part looks much better now 😄

moloko

Thanks for the updates, all looking pretty good from a review point of view aside from some final (minor) things.

I've now started testing for real (i.e. against an LMS - Moodle 1.9 purely because that's what I have installed on my PC but I'll try and find some time to check in SCORM Cloud too) and have been doing so by comparing the time your new code logs against what our existing code would do.

My reasoning behind this is that we've been using our code internally for around 8 years on many hundreds of e-learning courses (mainly SCORM1.2) and the same code is of course used in Adapt so will have been used on even more hundreds of courses built by the many people/companies who use Adapt.

in SCORM 1.2 it all looks fine to me, the only real difference I can see is that our code is slightly more precise i.e. it will log the time as 0000:00:03.46 whereas your code does 00:00:03 - but that seems fine to me.

In SCORM 2004, however, the differences are slightly bigger - here's a few examples...
Ours: PT3S.92S
Yours: PT4.3S

Ours: PT45S.60S
Yours: PT45.98S

Ours: PT2M34S.71S
Yours: PT2M35.02S

Certainly Moodle is not flagging the format of yours as being a problem so I'm guessing it's just showing the same information only in a slightly different way? Equally there will be a very slight difference in time due our code running first- it's on the line immediately before our code calls scorm.quit() so there shouldn't be much in it.

I did most of the testing in Firefox (open source ftw!!) but also did a quick check in IE11 and Chrome. All fine in those.

moloko · 2018-01-12T15:07:41Z

src/JavaScript/SCORM_API_wrapper.js

+    var s = "0" + dtm.getSeconds(); 
+    hms = h.substr(h.length - 2) + ":"+ m.substr(m.length - 2) + ":"; 
+    hms += s.substr(s.length - 2); 
+    return hms 


missing semicolon

moloko · 2018-01-12T15:09:48Z

src/JavaScript/SCORM_API_wrapper.js

@@ -33,11 +33,13 @@ pipwerks.SCORM = {                                  //Define the SCORM object
    version:    null,                               //Store SCORM version.
    handleCompletionStatus: true,                   //Whether or not the wrapper should automatically handle the initial completion status
    handleExitMode: true,                           //Whether or not the wrapper should automatically handle the exit mode
+    handleSessionTime: true,                        //Whether or not the wrapper should automatically handle the session time	


trailing space on the end of this line (and on the majority of lines you have added)

and just a thought but defaulting this to true could cause problems for anyone who goes to update to the latest version in their project... if they don't realised this functionality has been added then they run the risk of having two separate bits of code trying to set the session_time... I'll defer to @pipwerks as to what he thinks the default should be.

Changed to false, I do agree this way will allow a more "retrocompatible" upgrade.

moloko · 2018-01-12T15:10:14Z

src/JavaScript/SCORM_API_wrapper.js

+    var nH = Math.floor(nCs / 360000); 
+    nCs -= nH * 360000; 
+    var nMin = Math.floor(nCs /6000); 
+    nCs -= nMin * 6000 


missing semi colon

canvasplay · 2018-01-15T09:14:28Z

Hi @moloko!
Thanks for the review and the testing!

I have added the missing semicolons and fixed all the wired trailing spaces, also changed the default value for handleSessionTime, I think it makes sense to be false be default.

I'm glad to hear that everything is working properly in your testing!! aside from some final (minor) things 😄

I guess the amount of time differences your are getting are due to code execution delays as you mention, from my experience, SCORM request/response communication is not always speedlight.

I'm confused about the time format differences in SCORM 2004. I have checked the ISO and could not find any references to that double designator "S" for the seconds.

Yours: PT3S.92S
Mine: PT4.3S

Looks like yours is trying to distinguish seconds from centiseconds, so in this example your format is defining two separate values for seconds and using the same designator twice ("3S" and ".92S").
As I understand from the ISO spec the format should be number+designator... don't know 😕

Anyways I guess both formats are still valid.

My concern about your format is that I'm not sure what is the final value, depending on server's implementation I can figure out two possibilities, the two values get sum or maybe one of the two gets ignored.

What do you think?

moloko · 2018-01-15T12:08:48Z

I suspect your version is probably correct. Like I say, most of our experience is with SCORM 1.2 so I'm certainly not claiming that our code is correct, more just raising it as something to look at!

Maybe @pipwerks knows the answer...

Anyway I'm happy with all of this now and would be happy to have it go in as-is - but this repository belongs to @pipwerks so I will need to defer to him at this point as I can't take it any further.

canvasplay · 2018-01-15T14:23:37Z

Thanks so much for the review @moloko!
@pipwerks, I know you are a busy man, but if you could find some time to review this it would be wonderful 😄

moloko · 2018-01-15T17:48:23Z

@canvasplay @pipwerks please ignore everything I said about the SCORM 2004 time format - @canvasplay is absolutely correct to say 'format should be number+designator' i.e. PT4.3S and not PT4S.3S.

Turns out that the code I was comparing against had an old version of our 'convertToSCORM2004Time' function that had a bug in the time format that was causing an additional 'S' to be included.

The worst part is that I found, logged and fixed that bug - so I really should have known what the correct format is..!

pipwerks · 2018-01-17T08:19:14Z

Hi guys

Thanks for the work on this. I've been quiet on the thread, but I've been following your posts.

I haven't really talked about it much, but I wrote an entirely new wrapper years ago that included session time handling and a bunch of other features. Never released it because the wrapper was focused on improving CMI interaction handling, but most LMSs don't allow reporting on CMI interaction data fields, so why bother. I shelved it. :/

Anyway, the way I approached session timer was to add an option automate_session_timer, with a default of true. Mind you, this was a new wrapper so I wasn't worried about backwards compatibility. If extending the pipwerks SCORM wrapper, I'd definitely play it safe and have it default to false.

Regarding keeping exact time... session time handling will rarely align perfectly with time kept by the LMS, because the point at which the timer is started and the point at which it is stopped will vary. For example, an LMS like SumTotal will automatically start a session timer once the course window is opened, even if the SCORM API has not been initialized. (There is no rule that the SCORM API has to be initialized the second the window opens.) Some developers may choose to initialize the course after a user action, such as clicking a 'start' button. Likewise, the course may have its SCORM API terminated before the user closes the course window, but the LMS may keep tracking session time until the window is closed. So IMO it's safer to work under the assumption that time tracking is imperfect, and you can only do so much.

For kicks, here are excerpts from my code:

setLeadingZero = function (num){
    num = toInt(num);
    return (num < 10) ? num = "0" +num : num;
};


/* -------------------------------------------------------------------------
    formatTime()
    For session time handling
    Returns the time in the format expected by the SCORM API
    Supports both SCORM 1.2 and 2004.

    Parameters:  elapsed_milliseconds [number]
    Returns:     the time, in the proper format [string]
---------------------------------------------------------------------------- */

formatTime = function (elapsed_milliseconds){

    var milliseconds = 1000,
        seconds = (milliseconds * 60),
        minutes = (seconds * 60),
        hh = setLeadingZero(elapsed_milliseconds / minutes),
        mm = setLeadingZero((elapsed_milliseconds % minutes) / seconds),
        ss = setLeadingZero(((elapsed_milliseconds % minutes) % seconds) / milliseconds);

    return (version === "2004") ? "P" +hh +"H" +mm +"M" +ss +"S" : hh +":" +mm +":" +ss;

};


/* -------------------------------------------------------------------------
    getTimeStamp()
    For session time handling
    Returns a timestamp format expected by the SCORM API
    Supports both SCORM 1.2 and 2004.

    If date object is supplied as parameter, will convert that date to
    timestamp. If no date obj suppied, will default to current date/time.

    Parameters:  d [date object] (optional)
    Returns:     timepstamp [string]
---------------------------------------------------------------------------- */

getTimeStamp = function (d){

    if(!d){ d = new Date(); }

    /* SCORM 1.2 uses different format: CMITime
        A chronological 24 hour clock.
        Identified in hours, minutes, and seconds in the format:
        HH:MM:SS.S
    */

    var day = "",
        hours = setLeadingZero(d.getHours()),
        minutes = setLeadingZero(d.getMinutes()),
        seconds = setLeadingZero(d.getSeconds());

    if(version === "2004"){
        day = setLeadingZero(d.getFullYear()) +"-" +
              setLeadingZero(d.getMonth()) +"-" +
              setLeadingZero(d.getDate()) +"T";
    }

    return day +hours +":" +minutes +":" +seconds;

};


/* -------------------------------------------------------------------------
    getTime()
    For session time handling
    Returns the time portion of a date object.

    If date object is supplied as parameter, will return time portion of that
    date object. If no date obj suppied, will default to current time.

    Parameters:  d [date object] (optional)
    Returns:     time [string]
---------------------------------------------------------------------------- */

getTime = function (d){
    if(!d){ d = new Date(); }
    return d.setDate(d.getDate());
};


/* -------------------------------------------------------------------------
    getCurrentTime()
    For session time handling
    Utility function for retrieving current time.

    Parameters:  none
    Returns:     time [string]
---------------------------------------------------------------------------- */

getCurrentTime = function (){
    return getTime();
};


/* -------------------------------------------------------------------------
    startTimer()
    For session time handling
    Starts a timer for keeping track of session duration.
    You can retrieve the total time using getElapsedTime()

    Parameters:  none
    Returns:     none
---------------------------------------------------------------------------- */

startTimer = function (){
    session_time_start = getCurrentTime();
};


/* -------------------------------------------------------------------------
    stopTimer()
    For session time handling
    Stops the session timer.
    You can retrieve the total time using getElapsedTime()

    Parameters:  none
    Returns:     none
---------------------------------------------------------------------------- */

stopTimer = function (){
    if(session_time_start > 0){
        session_time_end = getTime();
    }
};


/* -------------------------------------------------------------------------
    getElapsedTime()
    For session time handling
    Returns the total session time by computing difference between
    session_time_start (as set by startTimer()) and session_time_end
    (as set by stopTimer()).

    Parameters:  none
    Returns:     elapsed time [string], auto-formatted for 2004 or 1.2
---------------------------------------------------------------------------- */

getElapsedTime = function (){
    return formatTime((session_time_start > 0 && session_time_end > 0) ? session_time_end - session_time_start : 0);
};

In the init routine:

//Option to automatically start session timer
if(automate_session_timer){ startTimer(); }

In the terminate routine:

//Option to automatically record session time
if(automate_session_timer && session_time_start !== 0){

    //End session timer
    stopTimer();
    success = setData(cmi_time, getElapsedTime());

            /*
            The LMS is responsible for adding up the session_times reported by each SCO launch
            and reporting the total time spent in the course via cmi.core.total_time/cmi.total_time

            From 2004 v3 docs:
            "Since a SCO is not required to set a value for this data model element (not required
            to keep track of the session time), an LMS shall keep track of session time from the time
            the LMS launches the SCO. If the SCO reports a different session time, then the LMS shall
            use the session time as reported by the SCO instead of the session time as measured by the LMS."
            */

        }

@canvasplay , your code is significantly different than mine (notably the csToISODuration) so I'd like to take some time to understand it before accepting the pull request. Thanks for all your work on this.

pipwerks · 2018-01-17T08:24:47Z

I just noticed I started work on the second wrapper in 2009 and last worked on it in 2011. Seven to nine years ago. Sheesh, time flies.

canvasplay · 2018-01-17T10:56:45Z

Hey @pipwerks!
Nice to know you are there 😄, but... wow! This was totally unexpected...
Maybe my contribution is now senseless... I just wanted to contribute to a project that I've been using for many years.

Is the new wrapper going to replace this one or are you planning to create a new one?
If I can help, just tell me 😄

Cheers!

pipwerks · 2018-01-18T04:23:44Z

No plans to officially release the other wrapper, though one of these days I might dust it off and post it for anyone interested.

canvasplay · 2018-01-19T11:07:18Z

Hi!

About your concern on understanding my csToISODuration method, I've been checking out your method and it looks like a simplified version of mine. In my humble opinion, mine is more complete and conformant with the iso, it does cover even years.
For more trustness, you can pass the tests i've created, @moloko have also tested the thing in a couple of LMSs and everything worked fine.

Regarding the other differences with your code, I am sorry but you are the only one who can see the whole thing cause the other wrapper is not online....

Differences apart, you will make me happy by accepting this pull request 😄 , and will make me feel like the effort was worth it.

Thanks so much!!

moloko · 2018-06-08T09:08:34Z

@pipwerks don't suppose you've had a chance to look at this have you? I think what @canvasplay has done would be a really useful addition...

moloko · 2018-08-14T17:16:04Z

👀

pipwerks · 2018-09-07T05:25:29Z

Geez, it has been almost a year since this was submitted. Sorry about that.

I have made some minor updates to the wrapper, including adding module support. However I am still wary of adding new features for fear of introducing bugs.

I am open to adding support for timers, but I will need to review again to see which path I feel comfortable with -- namely, which approach is the simplest and most reliable.

Thanks

moloko · 2018-09-07T09:08:42Z

@pipwerks there are some unit tests in place if you want to verify it all behaves as it should...

canvasplay · 2019-11-14T19:03:21Z

Hello! I'm back :D
Conflicts solved! Now what?

canvasplay added 4 commits October 11, 2017 14:39

added "handleSessionTime" bool flag

61593c2

Added dtmInitialized property in data object

b24152c

set value for scorm.data.dtmInitialized on pipwerks.SCORM.connection.…

c3eef37

…initialize call

Added UTIL methods for time format handling

4057be4

moloko reviewed Dec 21, 2017

View reviewed changes

remove commented out lines

21df630

canvasplay added 4 commits December 22, 2017 11:37

first approach to unit testing for the javascript part

e83689b

set qunit.js as unit testing engine

938d718

add some null like tests for time util functions

b334dc9

add tests for negative duration and fix the functions to pass them

31f70a3

moloko reviewed Jan 11, 2018

View reviewed changes

canvasplay added 6 commits January 12, 2018 08:27

fix typo 'subtract'

9401af2

move single comment into comment block

4b5763d

fix formatting for readability

cccb97a

rename time util functions to msToCMIDuration and csToISODuration

ab4e47b

Add reference to ISO code

a7d3bc1

add more relevant tests and descriptions

556ec5b

moloko reviewed Jan 12, 2018

View reviewed changes

Add a couple of missing semicolons and fix all trailing spaces

4cec5a6

Change default value for handleSessionTime to false

8b3e524

Merge branch 'master' into master

99e4400


		var dtm = new Date();

		//in the next line you substract the time recorded when initialising


		pipwerks.UTILS.MillisecondsToCMIDuration = function(n){

		//Convert duration from milliseconds to 0000:00:00.00 format

handleSessionTime #37

Are you sure you want to change the base?

handleSessionTime #37

Conversation

canvasplay commented Oct 17, 2017

moloko commented Oct 17, 2017 • edited Loading

moloko commented Oct 18, 2017

pipwerks commented Oct 21, 2017

moloko commented Oct 22, 2017

canvasplay commented Nov 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moloko commented Dec 21, 2017

canvasplay commented Dec 22, 2017

canvasplay commented Jan 2, 2018

Choose a reason for hiding this comment

moloko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canvasplay commented Jan 12, 2018

moloko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canvasplay commented Jan 15, 2018 • edited Loading

moloko commented Jan 15, 2018

canvasplay commented Jan 15, 2018 • edited Loading

moloko commented Jan 15, 2018

pipwerks commented Jan 17, 2018 • edited Loading

pipwerks commented Jan 17, 2018 • edited Loading

canvasplay commented Jan 17, 2018 • edited Loading

pipwerks commented Jan 18, 2018

canvasplay commented Jan 19, 2018

moloko commented Jun 8, 2018

moloko commented Aug 14, 2018

pipwerks commented Sep 7, 2018

moloko commented Sep 7, 2018 • edited Loading

canvasplay commented Nov 14, 2019

moloko commented Oct 17, 2017 •

edited

Loading

canvasplay commented Jan 15, 2018 •

edited

Loading

canvasplay commented Jan 15, 2018 •

edited

Loading

pipwerks commented Jan 17, 2018 •

edited

Loading

pipwerks commented Jan 17, 2018 •

edited

Loading

canvasplay commented Jan 17, 2018 •

edited

Loading

moloko commented Sep 7, 2018 •

edited

Loading