scorm-api-wrapper icon indicating copy to clipboard operation
scorm-api-wrapper copied to clipboard

handleSessionTime

Open canvasplay opened this issue 7 years ago • 23 comments

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.

canvasplay avatar Oct 17 '17 08:10 canvasplay

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 avatar Oct 17 '17 16:10 moloko

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

moloko avatar Oct 18 '17 09:10 moloko

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.

pipwerks avatar Oct 21 '17 23:10 pipwerks

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

moloko avatar Oct 22 '17 10:10 moloko

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

canvasplay avatar Nov 24 '17 09:11 canvasplay

@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...

moloko avatar Dec 21 '17 10:12 moloko

@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 avatar Dec 22 '17 10:12 canvasplay

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!

canvasplay avatar Jan 02 '18 15:01 canvasplay

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

canvasplay avatar Jan 12 '18 11:01 canvasplay

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?

canvasplay avatar Jan 15 '18 09:01 canvasplay

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.

moloko avatar Jan 15 '18 12:01 moloko

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 😄

canvasplay avatar Jan 15 '18 14:01 canvasplay

@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..!

moloko avatar Jan 15 '18 17:01 moloko

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 avatar Jan 17 '18 08:01 pipwerks

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.

pipwerks avatar Jan 17 '18 08:01 pipwerks

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!

canvasplay avatar Jan 17 '18 10:01 canvasplay

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

pipwerks avatar Jan 18 '18 04:01 pipwerks

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!!

canvasplay avatar Jan 19 '18 11:01 canvasplay

@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 avatar Jun 08 '18 09:06 moloko

👀

moloko avatar Aug 14 '18 17:08 moloko

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

pipwerks avatar Sep 07 '18 05:09 pipwerks

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

moloko avatar Sep 07 '18 09:09 moloko

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

canvasplay avatar Nov 14 '19 19:11 canvasplay