-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Implement a course outline REST API #2216
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,9 +100,10 @@ def course_handler(request, tag=None, package_id=None, branch=None, version_guid | |
| DELETE | ||
| json: delete this branch from this course (leaving off /branch/draft would imply delete the course) | ||
| """ | ||
| if 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'): | ||
| response_format = request.REQUEST.get('format', 'html') | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this necessary (vs. just checking HTTP_ACCEPT?).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the same logic I added when I implemented the assets JSON service. It makes it easier to test the service in a browser. Ideally we'd have support an URL like: http://localhost:8001/course/AndyA.AA101.2014/branch/draft/block/2014.json but that is hard to do until we implement something like the Python REST Framework. It also makes it easier on the client, as it isn't always straightforward to set the HTTP_ACCEPT header and have it passed through all the layers of backbone etc. |
||
| if response_format == 'json' or 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'): | ||
| if request.method == 'GET': | ||
| raise NotImplementedError('coming soon') | ||
| return JsonResponse(_course_json(request, package_id, branch, version_guid, block)) | ||
| elif request.method == 'POST': # not sure if this is only post. If one will have ids, it goes after access | ||
| return create_new_course(request) | ||
| elif not has_course_access( | ||
|
|
@@ -125,6 +126,37 @@ def course_handler(request, tag=None, package_id=None, branch=None, version_guid | |
| return HttpResponseNotFound() | ||
|
|
||
|
|
||
| @login_required | ||
| def _course_json(request, package_id, branch, version_guid, block): | ||
| """ | ||
| Returns a JSON overview of a course | ||
| """ | ||
| __, course = _get_locator_and_course( | ||
| package_id, branch, version_guid, block, request.user, depth=None | ||
| ) | ||
| return _xmodule_json(course, course.location.course_id) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should pass the locator so you don't have to figure it out again in _xmodule_json. Oh, I see that your method is recursive. If not passing in locator, you can use underscore to indicate that the variable is not used. |
||
|
|
||
|
|
||
| def _xmodule_json(xmodule, course_id): | ||
| """ | ||
| Returns a JSON overview of an XModule | ||
| """ | ||
| locator = loc_mapper().translate_location( | ||
| course_id, xmodule.location, published=False, add_entry_if_missing=True | ||
| ) | ||
| is_container = xmodule.has_children | ||
| result = { | ||
| 'display_name': xmodule.display_name, | ||
| 'id': unicode(locator), | ||
| 'category': xmodule.category, | ||
| 'is_draft': getattr(xmodule, 'is_draft', False), | ||
| 'is_container': is_container, | ||
| } | ||
| if is_container: | ||
| result['children'] = [_xmodule_json(child, course_id) for child in xmodule.get_children()] | ||
| return result | ||
|
|
||
|
|
||
| @login_required | ||
| @ensure_csrf_cookie | ||
| def course_listing(request): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't think this test has any sections in the course (any children). You should probably add some content to the course to make it more interesting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Somehow I thought that this was a toy course as the other tests seemed to be doing interesting things with it. I would have noticed if it wasn't so painful to set up the debugger. I'll manually add content as you suggest, and then I can verify exactly what is returned too.