401 - Not implemented error handling

[danielghost]

Currently the SCORM wrapper checks support for non-mandatory elements via an LMSGetValue/GetValue before attempting an LMSSetValue/SetValue. For elements with _children, such as cmi.interactions and cmi.objectives, it assumes that if the _count is supported, all elements of that data model are supported. Whilst this is generally true, there are edge cases where an LMS doesn't support all child elements, causing Adapt to raise an error.

For future reference the following occurs for Adobe Learning Manager:

SCORM 1.2

• cmi.student_preference.language is unsupported by the LMS, but incorrectly returns a 201 invalid element error rather than the 401 unsupported error. The error codes are inconsistently implemented with an LMSGetValue call to this element incorrectly returning 201, but an LMSSetValue correctly returning 401. cmi.student_preference._children correctly returns an empty string and a 401 error.
• cmi.objectives._children returns “id,score,status” as the supported elements, but score and status both return 401 errors when you attempt to use them.

SCORM 2004

• cmi.learner_preference and cmi.objectives have not been implemented, yet are mandatory elements. Both incorrectly return “undefined” and a 201 error when calling _children.

Should we simplify this approach and remove conditional checks completely and instead handle 401 errors silently? This removes the potential for implementation issues such as #294, with the knowledge that a call to any valid unsupported elements will fail gracefully in the same way they are when isSupported calls are made. If we wish to maintain conditional checks to reduce the number of 401 handled errors, the recommended approach would be to check which _children are returned first - see #241.

[oliverfoster]

Do we know in which systems the isSupported method is currently working correctly? I'm a little anxious about removing, it doesn't seem to form part of or change the outcome of the 404 solution for the Adobe LMS.

[danielghost]

It resolves #241 as that issue was actually caused by the LMS incorrectly returning a 201 error for that unsupported element via LMSGetValue, but correctly returning a 401 error when using LMSSetValue. As isSupported calls are using LMSGetValue to first check support, the error was being raised. This is obviously an edge case as it is due to an incorrect LMS implementation, which still hasn't been resolved despite it being rebranded to Adobe Learning Manager since that issue was raised.

Despite this, the #303 PR is essentially still using the same logic as isSupported, but removing the need to make an additional call first, instead checking it directly for the element as required. It seems a more robust way of ensuring 401 errors are always correctly handled without raising an error to the learner for unsupported data elements. As above, if we wish to reduce the number of calls being made to unsupported elements, we can change the isSupported logic to check which specific elements are supported first, but it seems unnecessary as it is up to the SCO to determine how to handle the returned LMS error codes. There should be no detrimental effect on the LMS when calling a valid unsupported element.

[oliverfoster]

PR is essentially still using the same logic as isSupported

It's not the same logic.

but removing the need to make an additional call first

This is how the spec says to check. It has worked until this LMS. It's not an additional call, it's the correct checking call that has been there for a long time. SCORM_1.2_RunTimeEnv (section 3-9)

This is obviously an edge case as it is due to an incorrect LMS implementation.

Yes, you're changing the way support detection is working for all lmss because one isn't implemented correctly.

It seems a more robust way of ensuring 401 errors are always correctly handled

Have you tested it on lmss that don't support a feature but which do report that they don't support a feature correctly?

As above, if we wish to reduce the number of calls being made to unsupported elements

For me it's not about the number of calls. It's about unnecessarily removing a system which works to spec, for an untested alternative. There is a risk that by dropping errors on a set and performing a set on an indicated unsupported property, we open ourselves up to other as yet unidentified oddities on a set for a different reason from any of the other lmss (as is Matts concern). As far as I can tell, both systems can be implemented together, and there is no reason to remove the isSupported checks - you're just returning early after the request in a particular circumstance.

[danielghost]

The section quoted above doesn't explicitly say you must use LMSGetValue/GetValue to check if an element is supported, but that it can be used to determine which child elements of a model are supported:

datamodel.element._children The _children keyword is used to determine all of the elements in a group or category that are supported by the LMS.

LMSGetValue(datamodel.group._children) The return value is a comma-separated list of all of the element names in the specified group or category that are supported by the LMS. If an element has no children, but is supported, an empty string ("") is returned. An empty string ("") is also returned if an element is not supported. A subsequent request for last error [LMSGetLastError()] can determine if the element is not supported. The error “401 Not implemented error” indicates the element is not supported.

There would be little point in including a 401 error code as part of the spec for LMSSetValue/SetValue if you had to check it was supported first.

After discussion, the decision has been made to include the commits for the 401 error checking when handling errors, but keep the isSupported calls as a precaution, with a separate PR made to change the supported logic accordingly from _count to _children.

[github-actions[bot]]

:tada: This issue has been resolved in version 5.9.4 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: