search

jazkarta / edx-platform-for-isc

fork of the edx platform for development w/ isc
GNU Affero General Public License v3.0
1 stars 2 forks source link

ISC Customizations: Improvements to LTI xmodule #10

Open cewing opened 9 years ago

cewing commented 9 years ago

Preserve the changes made in the following two commits:

If possible, convert the lti_spec_namespace which is currently hard-coded to a configurable field that defaults to the value set in the original EdX code (http://www.imsglobal.org/services/ltiv1p1/xsd/imsoms_v1p0) and then use that field value when appropriate.

Ensure that the tests added to cover this change are preserved and updated as required to make them work properly.

cewing commented 9 years ago

@natea: this seems like the type of thing that ought to be made a PR against EdX. Making the LTI namespace configurable would allow the block to work with LTI Providers that don't use the "standard" namespace (like SCORMCloud). Even in the light of a pending 'scorm-cloud' xblock, these changes would be useful (and are non-harmful to the existing implementation, I think).

natea commented 9 years ago

@cewing agreed.

bryanlandia commented 9 years ago

I've made a pull request to 'isc-birch-patches' branch of edx-platform and a related push to birch branch of ISCLC/ISC (edx configuration). Have to get my head around how I want to do the PR to edx master, though.

cewing commented 9 years ago

@bryanlandia: I've commented on the PR, can you take a look there? @natea: you too, please.

natea commented 9 years ago

@nedbat can you remind us how we could add a dictionary with coursewide parameters, so we don't have to override the LTI namespace directive to each instance of the LTI xmodule?

cpennington commented 9 years ago

@natea there's the SettingsService, which reads out of django settings. It's not per-course, though.

Otherwise, until we get something that functions as a course-scope, your best bet is probably to use an inherited field to allow the field to be set on the course, and then to flow down to the LTI xblock.

cewing commented 9 years ago

@cpennington, so this would be a field in the course settings, perhaps advanced settings or the like, which would be then inherited by blocks within the course?

bryanlandia commented 9 years ago

@natea Turns out SettingsService wasn't implemented for XBlocks until after the official Birch release. Weird thing is that the release's version of xblockutils has mixins for XBlock that are set up to use SettingsService. I'm going to do a bit more looking to see if we should just grab the new xmodule.services code with SettingService for our edx-platform 'fork' or go a different route.

natea commented 9 years ago

Thanks for the update Bryan. With the Cypress release just around the corner, it might be best to wait for that since it'll have the SettingsService for sure.

bryanlandia commented 9 years ago

@natea - OK, so put this one on hold? Will the work on this remain part of the Jazkarta contract?

bryanlandia commented 9 years ago

The code in the PR as is has been deployed to staging. I'm placing this Issue on hold to revisit when we upgrade ISC to Cypress. At that point we can look again at making the namespace spec URL configurable per course/XBlock instance.