pipwerks / scorm-api-wrapper

The pipwerks SCORM API Wrapper
http://pipwerks.com
357 stars 125 forks source link

handleSessionTime #37

Open canvasplay opened 6 years ago

canvasplay commented 6 years ago

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.

moloko commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

canvasplay commented 6 years ago

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

moloko commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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

canvasplay commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

👀

pipwerks commented 6 years ago

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 commented 6 years ago

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

canvasplay commented 4 years ago

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