Add a compatibility function for get_course_lti_endpoints - #22
Conversation
e59b347 to
c358205
Compare
c358205 to
d4bae5e
Compare
| This function is called by get_course_lti_endpoints when using LTI result service to | ||
| discover the LTI result endpoints. It is similar to outcome_service_url property but | ||
| with a mapping for backward compatibility. | ||
|
|
|
👍 This looks great. Thanks for the contribution. @robrap Would you be able to review this? |
d4bae5e to
99c8d34
Compare
| 'grade_handler': 'outcome_service_handler', | ||
| 'lti_2_0_result_rest_handler': 'result_service_handler' | ||
| } | ||
| return self.runtime.handler_url(self, mapping[service_name], thirdparty=True).rstrip('/?') |
There was a problem hiding this comment.
Would it make sense to add a result_service_url call and instead of mapping to the service name, map to the function itself? This would remove a little code redundancy, take advantage of the comments in the outcome_service_url property, and would have a little more consistency around the implementation. Thoughts?
There was a problem hiding this comment.
Not sure if I understand correctly. The get_outcome_service_url function was missing and it was called from https://github.com/edx/edx-platform/blob/master/lms/djangoapps/courseware/views/views.py#L1075. Sorry, not sure if I understand your comment. The get_outcome_service_url was missing from lti_consumer (was throwing exception when the endpoint was called). It is called from here: https://github.com/edx/edx-platform/blob/master/lms/djangoapps/courseware/views/views.py#L1075. The service_name parameter was specific for LTI xmodule functions (grade_handler and lti_2_0_result_rest_handler). During the refactor to xblock, those function names changed. So that's why we need a mapping.
There was a problem hiding this comment.
I guess I wasn't clear about my refactor request. All looks good, so this is a minor request. I'll explain more soon, but we have hackathon followed by a holiday on Monday. I hope early next week will be fine.
There was a problem hiding this comment.
Sorry I forgot about this. Here is untested code to explain what I meant. I still feel new to Python, so not sure if there are any issues with this.
def get_outcome_service_url(self, service_name="grade_handler"):
"""
This function is called by get_course_lti_endpoints when using LTI result service to
discover the LTI result endpoints.
"""
mapping = {
'grade_handler': outcome_service_url,
'lti_2_0_result_rest_handler': result_service_url, # new function needed
}
return mapping[service_name]()
I removed the docstring comment about outcome_service_url, because this function now just calls the appropriate property, which has its own comments.
Does this make my original comment any more clear? Does this seem reasonable @xcompass ? @douglashall?
There was a problem hiding this comment.
Thanks. I see what you mean. Updated. How does it look now?
There was a problem hiding this comment.
Thank @xcompass. That's what I had in mind. This is a small PR, but generally we don't squash commits until after the review so we can see the individual changes.
026fa3c to
1894542
Compare
| """ | ||
| Test `get_outcome_service_url` calls with service name lti_2_0_result_rest_handler | ||
| """ | ||
| handler_url = 'http://localhost:8005/outcome_service_handler' |
There was a problem hiding this comment.
outcome_service_handler => result_service_handler
|
@xcompass: I had one minor typo fix for you. Feel free to fix and amend/squash this minor change. |
1894542 to
43ee768
Compare
This allows this xblock being hooked into get_course_lti_endpoints for discovering LTI result/outcome endpoints.
|
@xcompass @robrap @douglashall Should we go ahead and merge this or close it out? |
|
I would be happy to see it's merged :) |
|
@robrap @douglashall Is there anything additional required for this to be merged, from your perspective? |
|
Actually, I'm going to merge this one. What I am going to try to make happen as soon as possible is tagging it and getting it into edx-platform along with the other PR. |
|
@xcompass: Someone merged another PR to xblock-lti-consumer and bumped the version to 1.1.1, but there is no release tag and it hasn't been updated in edx-platform. So, I need to get that resolved before being able to release this change. If interested, you can follow this conversation about releases here: #24 (comment) |
5 participants
This allows this xblock being hooked into get_course_lti_endpoints for
discovering LTI result/outcome endpoints.
Background: The old xmodule supports enumerates LTI endpoints so that they can be discovered and consumed by external tools. However, this functionality is missing when xmodule was refactored into xblock. Currently, only the old LTI xmodules can be discovered when /courses/COURSE_ID/lti_rest_endpoints/ is called.
Testing:
The endpoints for LTI component should be listed in JSON format.
This PR is needed by https://github.com/edx/edx-platform/pull/13639 to enable the endpoints to query both old xmodules and new xblocks.