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

Outcome service bug fix and other changes #45

Closed
wants to merge 5 commits into from

Conversation

ktonon
Copy link
Contributor

@ktonon ktonon commented Apr 17, 2015

Each commit is a separate item. Three are related to outcome service. The last is to ensure that editors obey the 2 space indentation adopted in the project.

I'm using ims-lti outcome service to post grades to a server that uses a self-signed certificate. The ca option on https.request makes it possible to do this.

Also, I'm posting the grades long after the launch request. Currently I'm storing parts of the original request, so that I can call provider.parse_request to in turn create the outcome service. I'd like to refactor the outcome service constructor to make it easier to create directly.

Last, a small bug fix. https.request should be used for ssl

This change to the constructor makes it a bit easier to instantiate OutcomeService directly.

Before you would have to either

* store parts of the original request to create a provider
* construct a mock provider to pass to the outcome service
@ttsirkia
Copy link

Looks brilliant! Espcially that Outcome can easily be used afterwards without creating a fake request.

@omsmith
Copy link
Owner

omsmith commented Apr 24, 2015

Thanks for the contribution. I'll have time to go over it tonight.

I'll look for myself, but is this breaking at all, or would it fit within a minor version?

@ktonon
Copy link
Contributor Author

ktonon commented Apr 24, 2015

The change in signature to the OutcomeService constructor is breaking, if anybody is already using it directly.

@omsmith
Copy link
Owner

omsmith commented Jun 20, 2015

Landed as 5be2259 6815b6c in v2.1.6 and ad0db90 a6032e3 5e590ff in v3.0.0

@omsmith omsmith closed this Jun 20, 2015
@omsmith
Copy link
Owner

omsmith commented Jun 20, 2015

Many apologies for not getting to this earlier - I'm sure it was quite annoying.

@ktonon
Copy link
Contributor Author

ktonon commented Jun 21, 2015

No worries! Thanks for merging

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

Successfully merging this pull request may close these issues.

None yet

3 participants