search

adlnet / xAPIWrapper

Wrapper to simplify communication to an LRS
https://adlnet.gov/projects/xapi/
Apache License 2.0
214 stars 114 forks source link

ADL.xapiutil.getActorId #145

Closed oliverfoster closed 4 years ago

oliverfoster commented 5 years ago

getActorIdString and getActorID seem to process the id in two different ways.


    ADL.xapiutil.getActorId = function (actor) {
        return actor.mbox || actor.openid || actor.mbox_sha1sum || actor.account;
    };

    ADL.xapiutil.getActorIdString = function (actor) {
        var id = actor.mbox || actor.openid || actor.mbox_sha1sum;
        if (!id) {
            if (actor.account) id = actor.account.homePage + ":" + actor.account.name;
            else if (a.member) id = "Anon Group " + actor.member;
            else id = 'unknown';
        }
        return id;
    };

getActorIdString seems to take account of actor.member but getActorId does not. Is this an oversight or is it intended?

vbhayden commented 5 years ago

I think it's more of a poor naming choice. getActorId is only ever used in a display-only context, even by getObjectId which itself is only used for displaying something.

Really, getActorId as a name doesn't make sense at all given the possibility of an account IFI-type, as that only attempts uniqueness as a homePage / name pairing. We use Keycloak UUID's for name with the TLA implementations, but the spec doesn't enforce any formatting -- "The unique id or name used to log in to this account".

I'd almost rather getActorId just be replaced with getActorIdString, since that's a more intuitive definition.

oliverfoster commented 5 years ago

Cool. Same with getObjectId > getObjectIdName ?

I can alias them and output a deprecated warning for the original named function calls.