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

Use XBlock handlers in the LMS ... #1531

Merged
merged 3 commits into from
Nov 8, 2013

Conversation

cpennington
Copy link
Contributor

... and use them to implement ajax_url for XModule compatibility

@cpennington
Copy link
Contributor Author

@nedbat, @sarina: Review?

@sarina
Copy link
Contributor

sarina commented Oct 30, 2013

looks like a lot of XModule unit tests are failing... eek

@cpennington
Copy link
Contributor Author

Yeah, I'm working on that. The set of changes should be small, though.

render.handle_xblock_callback(
None,
'dummy/course/id',
quote_slashes('invalid Location'),
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like using quote_slashes is unnecessary for the string 'invalid Location'

@sarina
Copy link
Contributor

sarina commented Oct 30, 2013

Did a first-pass and it looks pretty good. Will do a more thorough pass when tests pass. Also consider having someone from Studio look at this - there are some CMS changes here.


descriptor = modulestore().get_item(location)
instance = load_preview_module(request, preview_id, descriptor)
instance = load_preview_module(request, descriptor)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm just not up on how the CMS will use handlers: is it a given that a handler in the CMS will imply "preview"? Or is this a misleading use of the word "preview"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, you're right, this is preview only, and should be named that way.

@nedbat
Copy link
Contributor

nedbat commented Oct 30, 2013

Take care of a few things, and then 👍

'input_[]_3': 'test',
'input_4': None,
# 'input_5' intentionally left unset,
'input_6': 5})
Copy link
Contributor

Choose a reason for hiding this comment

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

This multiline split doesn't seem right (closing paren on final line; would be neater to put the first dictionary entry on line 336 not 335) - split the same way it's split in test_peer_grading.py l 33

Why is input 5 intentionally left unset? I might prefer that in a comment above the declaration of valid_get_dict

self.module = SelfAssessmentModule(get_test_system(), self.location,
system = get_test_system()
system.xmodule_instance = Mock(scope_ids=Mock(usage_id='dummy-usage-id'))
self.module = SelfAssessmentModule(system, self.location,
Copy link
Contributor

Choose a reason for hiding this comment

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

system & self.location on their own lines

@sarina
Copy link
Contributor

sarina commented Oct 31, 2013

Unit tests for the new functions in lms/xblock/runtime.py are necessary, as well as a few comments. After that, 👍

@@ -89,6 +89,7 @@ transifex-client==0.9.1

# Used for testing
coverage==3.6
ddt==0.4.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jtauber: This library is LGPL. I'm going to merge this w/o waiting for your OK, but let me know if we need to, and we can pull it out. It's only actually used during tests.

@sarina
Copy link
Contributor

sarina commented Nov 1, 2013

If you rebase you should be able to fix the quality build - I merged #1554 yesterday.

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.

4 participants