Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to load activity_id from query string instead of config #152

Open
deepinder19 opened this issue Apr 2, 2019 · 3 comments
Open

How to load activity_id from query string instead of config #152

deepinder19 opened this issue Apr 2, 2019 · 3 comments
Assignees

Comments

@deepinder19
Copy link

_initFromQueryString in "TinCanJS/build/tincan-node.js" loads activity_id from url and creates a new Tincan.activity object. But if activity details are found in config, new activity object is created using those details and it overrides the activity object previously created using the url parameters.

Is there a way to let the url parameters set the activity object, or is it intended to be overridden by config?

@reedmclean
Copy link

It makes sense to me that the we should wait to call _initFromQueryString until after we've prepopulated the activity with the rest of the contents of cfg.

We could just move this:

TinCanJS/src/TinCan.js

Lines 152 to 154 in 8733f14

if (cfg.hasOwnProperty("url") && cfg.url !== "") {
this._initFromQueryString(cfg.url);
}

down below this:

TinCanJS/src/TinCan.js

Lines 185 to 187 in 8733f14

if (cfg.hasOwnProperty("registration")) {
this.registration = cfg.registration;
}

@brianjmiller, you probably understand more of the context on this project than I do. Do you have any problem with this? Was the intention here to let the config overwrite the URL parameters?

@deepinder19
Copy link
Author

@brianjmiller with current implementation I am not able to overwrite "activity" using url parameters but looking at the code its looks like it was intended to have the ability to overwrite activity from the url. This looks like a bug to me.

It looks like a simple change and I'd be happy to make a pull request if you can confirm. Thanks 🙂

@garemoko
Copy link
Contributor

garemoko commented Apr 9, 2019

I guess this comes down to a question of who gets to decide what the activity id is, the content author or the launching system.

The strongest argument I can see in favor of having the launch parameter overwrite the config is that cmi5 comes down in favor of the launching system deciding the activity id:

When the Object is the AU, the value of the Object's "id" property for a given AU MUST match the activityId defined in the launch URL

https://github.com/AICC/CMI-5_Spec_Current/blob/quartz/cmi5_spec.md#94-object

But on the other hand, prioritizing the config value is the current behavior, meaning this could be a breaking change for anybody relying on that behavior. I don't know if we have any way of identifying if anybody is doing this.

A middle route would be to keep the current behavior, but add an additional config setting that would allow the launch parameter to take priority if provided, and fall back to the config value if not.

Probably it's cleanest to follow cmi5 and update the version number of TinCanJS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants