resolve issue 143: dangling url params causing rejected state requests

[vbhayden]

https://github.com/adlnet/xAPIWrapper/issues/143

Removed this block from the getLRSObject body:

if (Object.keys(qsVars).length !== 0) {
    lrs.extended = qsVars;
}

Also added a simple test to check the getState and sendState functions, with it having failed when extra arguments were present.

[vbhayden]

@cawerkenthin does the version in this PR resolve the issue for you?

@brian-learningpool for your LMS-specific resolution as commented here:

//Add extended LMS-specified values to the URL
if (lrs !== null && lrs.extended !== undefined) {
    extended = new Array();
    for (prop in lrs.extended) {
        extended.push(prop + "=" + encodeURIComponent(lrs.extended[prop]));
    }
    if (extended.length > 0) {
        url += (url.indexOf("?") > -1 ? "&" : "?") + extended.join("&");
    }
}

does this branch impede your functionality? I'm not sure why browser url params were being included in the actual LRS request, but wanted to check that this resolution isn't too heavily handed.

[brian-learningpool]

Hi @vbhayden, thank you for tagging me.

The original issue came to light while a customer was testing against Saba Discovery. Unfortunately we don't have an in-house installation of that platform to test against, but I can reach out to the customer to verify that this change won't cause any problems, if you could hold off on merging it for a little while?

[cawerkenthin]

I do not believe this resolves the issue. The property lrs.extend is set in getLRSObject sets lrs.extended to include any property appearing in the command line (for some unknown reason). Later, in ADL.XHR_request the values of lrs.extended are added to the command line. This is a spec violation. An LMS cannot simply add any values it likes to a command line to xAPI content expect to receive those back. The every fact that there is a comment referrring to LMS should be an indication that the issue is LMS specific and has nothing to do with xAPI. The concept of handing parameters automatically for content is misguided.

Art Werkenthin RISC, Inc. @AWerkenthin 281-480-7910

---

From: Brian Quinn notifications@github.com Sent: Monday, March 11, 2019 4:43 AM To: adlnet/xAPIWrapper Cc: Werkenthin Art; Mention Subject: Re: [adlnet/xAPIWrapper] resolve issue 143: dangling url params causing rejected state requests (#144)

Hi @vbhaydenhttps://github.com/vbhayden, thank you for tagging me.

The original issue came to light while a customer was testing against Saba Discovery. Unfortunately we don't have an in-house installation of that platform to test against, but I can reach out to the customer to verify that this change won't cause any problems, if you could hold off on merging it for a little while?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/adlnet/xAPIWrapper/pull/144#issuecomment-471469855, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AD1z789MmGfmKkEvs2ZE75jhm-TVbtpLks5vViUlgaJpZM4bl6--.

[vbhayden]

This PR removes that block out in that getLRSObject body:

// iniitializes an lrs object with settings from
// a config file and from the url query string
function getLRSObject(config)
{
    var lrsProps = ["endpoint","auth","actor","registration","activity_id", "grouping", "activity_platform"];
    var lrs = new Object();
    var qsVars, prop;

    qsVars = parseQueryString();
    if (qsVars !== undefined && Object.keys(qsVars).length !== 0) {
        for (var i = 0; i<lrsProps.length; i++){
            prop = lrsProps[i];
            if (qsVars[prop]){
                lrs[prop] = qsVars[prop];
                delete qsVars[prop];
            }
        }
        // if (Object.keys(qsVars).length !== 0) {
        //     lrs.extended = qsVars;
        // }

        lrs = mergeRecursive(config, lrs);
    }
    else {
        lrs = config;
    }

    return lrs;
}

Merging something LMS-specific was an oversight that shouldn't have gone through and will be addressed.

[cawerkenthin]

Awesome, I will grab the fixed copy to continue work on the cmi5 library for ADL cmi5 Working Group.

Art Werkenthin RISC, Inc. @AWerkenthin 281-480-7910

---

From: Trey Hayden notifications@github.com Sent: Monday, March 11, 2019 8:59 AM To: adlnet/xAPIWrapper Cc: Werkenthin Art; Mention Subject: Re: [adlnet/xAPIWrapper] resolve issue 143: dangling url params causing rejected state requests (#144)

This PR removes that block out in that getLRSObject body:

// iniitializes an lrs object with settings from // a config file and from the url query string function getLRSObject(config) { var lrsProps = ["endpoint","auth","actor","registration","activity_id", "grouping", "activity_platform"]; var lrs = new Object(); var qsVars, prop;

qsVars = parseQueryString();
if (qsVars !== undefined && Object.keys(qsVars).length !== 0) {
    for (var i = 0; i<lrsProps.length; i++){
        prop = lrsProps[i];
        if (qsVars[prop]){
            lrs[prop] = qsVars[prop];
            delete qsVars[prop];
        }
    }
    // if (Object.keys(qsVars).length !== 0) {
    //     lrs.extended = qsVars;
    // }

    lrs = mergeRecursive(config, lrs);
}
else {
    lrs = config;
}

return lrs;

}

Merging something LMS-specific was an oversight that shouldn't have gone through and will be addressed.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/adlnet/xAPIWrapper/pull/144#issuecomment-471549478, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AD1z72uUh_Qv-FQwJi-M0FlJYnU9FUG5ks5vVmFGgaJpZM4bl6--.

[brian-learningpool]

Hi guys, to clarify this wasn't necessarily something which was LRS specific per se. It was observed on an LRS setup which happened to be reliant on extended parameters -- this was one LRS out of any number. I'm testing via emailing test packages to a customer so I don't have access to the exact environment at present but I will follow up and ascertain why.

However, initial reports are that this change has broken something which was previously working. I think merging this as it was -- essentially by commenting out a block of code -- was not ideal. When changing current behaviour it appears there was no consideration given to debug output such as a deprecation warning or providing a method of preserving the existing behaviour, if required.

[vbhayden]

Normally, I would agree that a deprecation warning would be the way to go, but adding potentially non-conformant URL parameters to the resource endpoint was breaking core functionality.

An alternative would be that we allow a config property to specify whether those URL parameters should be carried into the resource endpoint. ATM, this feels like the best solution for all parties. Frankly, I don't know why URL parameters were originally relevant at all to this wrapper, but, seeing as there's a 2016 commit for parsing them, they were on someone's mind.

Since this was a breaking change for your folks, it does feel unfair for it to happen suddenly so I've created a branch that preserves that behavior: https://raw.githubusercontent.com/adlnet/xAPIWrapper/carry-params/dist/xapiwrapper.min.js

[cawerkenthin]

@brian-learningpool The change that was originally made did not conform to the xAPI spec, and it was LRS specific. I think the ADL library should conform to the spec. I suspect your LMS vendor does not have a conformant LRS if they are accepting random parameters.

[brian-learningpool]

@cawerkenthin and @vbhayden , I'm still trying to get to the bottom of the Saba issue, however communication via email and the time difference with the client means this is slow going.

I think this might be a question of something that was potentially not invalid on a previous version of the spec, e.g. v0.9. There's an old issue on another similar repo from years ago which is very similar to what you have reported here: https://github.com/RusticiSoftware/TinCanJS/issues/57

If (hypothetically) this is the case for something which was acceptable in v0.9, but not in v1.0.3, how best do we handle any future (breaking) changes like this? I'm not familiar enough with the spec to know what such changes have taken effect between revisions, but the wrapper itself still mentions that it's based on v1.0.1 (https://github.com/adlnet/xAPIWrapper/blob/master/src/xapiwrapper.js#L263).

[cawerkenthin]

@vbhayden @brian-learningpool You may be correct about .9. Although we implemented at .9, I was not really involved until the 1.0 release. It is my understanding from others that .9 and 1.0 had quite a few differences. From my reading of the Rustici issue, it looks like the extended parms must be explicitly enabled. I do agree with Trey that the handling of URL parameters at all should not be part of this library... but we are probably stuck with it.

[brian-learningpool]

Likewise v0.9 was before I started getting involved in xAPI properly myself.

@vbhayden and @cawerkenthin, would you be open to a PR for a flag which allows explicitly enabling extended params? This could be off by default, i.e. extended parameters are dropped, but it would at least give those of us who are implementing solutions for customers on LMSs who are lagging in support for the later xAPI versions a means of still supporting them using this library.

[vbhayden]

Yeah, I think that's the best play given the circumstances.

Do note that changeConfig doesn't actually reuse the getLRSObject function, so that will need to change from just using mergeRecursive in order to act on whatever property is supplied to indicate inclusion of URL parameters.