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

1.1 and 1.1.1 compliance #19

Merged
merged 10 commits into from
Oct 22, 2014
Merged

1.1 and 1.1.1 compliance #19

merged 10 commits into from
Oct 22, 2014

Conversation

Tape
Copy link
Collaborator

@Tape Tape commented Sep 10, 2014

See #12. This will keep us fully compatible with 1.0 with fairly minimal code additions. 2.0 will be a completely different beast. A note for the Outcomes service, it is implemented exactly as the implementation guide has it specified and as a result some of the platforms have unexpected behavior (Moodle is a good example, they return the sourcedid as a JSON formatted string and expect you to return the hash attribute). If the platform follows the spec it should work fine.

A side note, I unit tested absolutely everything I possibly could while adding these updates but the outcomes service obviously needs a test bed, for now it does work to spec and I tested it against platforms. It would probably be a good idea down the line to make a test utility that can verify incoming data and present a "fake" server that will respond with the correct responses.

@coveralls
Copy link

Coverage Status

Coverage decreased (-19.09%) when pulling b64fbac on catturavideo:1.1 into d3daafb on omsmith:master.

@omsmith
Copy link
Owner

omsmith commented Sep 10, 2014

Cool to see. I'll try to have a look over it more in depth tonight, but initial pass looks alright.

Figuring out the testing story would be great for sure

@omsmith
Copy link
Owner

omsmith commented Sep 16, 2014

Uhg. Apparently "this evening" means, "almost a week from now" - sorry.

Thanks for doing the work and working on rounding out the LTI story here.

Before considering the Outcomes service, could you open two other pull requests? I'd like to see d6f9fae in one on its own and 1e1cf47, 83a207f and d94559c in another. This will let me merge those with a patch version bump and get it out to more users.

@Tape
Copy link
Collaborator Author

Tape commented Oct 3, 2014

I made those PRs, if you need I can make new one with all of the 1.1/1.1.1 updates after you merge in those two. I'll be a little more responsive this next time, got extremely busy prepping for a product launch.

Thanks!

@omsmith
Copy link
Owner

omsmith commented Oct 4, 2014

Cool - can we rebase this onto master now and see where we stand?

@Tape
Copy link
Collaborator Author

Tape commented Oct 4, 2014

Yessir, I'll give it a whirl when I get home
On Oct 4, 2014 11:44 AM, "Owen Smith" notifications@github.com wrote:

Cool - can we rebase this onto master now and see where we stand?


Reply to this email directly or view it on GitHub
#19 (comment).

@omsmith
Copy link
Owner

omsmith commented Oct 5, 2014

Great thanks for doing that - I'll try to look at it before I head to bed

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.63%) when pulling 22699f8 on catturavideo:1.1 into 8882003 on omsmith:master.

@Tape
Copy link
Collaborator Author

Tape commented Oct 14, 2014

I had some free time this afternoon/evening to implement a test server to handle the testing of outcomes. The test server validates the crpyto, XML structure, and numerical ranges of values.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.59%) when pulling 9773117 on catturavideo:1.1 into 8882003 on omsmith:master.


# Generate a unique identifier and apply the version to the header information
@head.ele 'imsx_version', 'V1.0'
@head.ele 'imsx_messageIdentifier', uuid.v1()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the spec call for v1 uuids?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope they're just treated as a unique ID. It could be as simple as a simple increment counter if you wanted it to be. I just went with UUID due to it's widespread use and guarantees.

@omsmith
Copy link
Owner

omsmith commented Oct 22, 2014

Awesome - it all seems pretty reasonable.

Is this going to require a major version bump or just a minor?

Either way probably good to bump to 1.0 sometime soon.

@Tape
Copy link
Collaborator Author

Tape commented Oct 22, 2014

I would consider this a minor bump at the very least - we could probably go with 1.0.0 though since this puts us at full compliance with the most widely used version.

Tape pushed a commit that referenced this pull request Oct 22, 2014
1.1 and 1.1.1 compliance
@Tape Tape merged commit 534f91c into omsmith:master Oct 22, 2014
@Tape Tape deleted the 1.1 branch October 22, 2014 20:08
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

Successfully merging this pull request may close these issues.

None yet

3 participants