-
Notifications
You must be signed in to change notification settings - Fork 7
(WIP) Content library code for upstream PR - Internal review #5
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
14df787
fac5060
46edb28
063ff7e
1d80203
74bf813
ed57db2
ccfbb64
5ba66aa
a6a8c32
7e94151
7dc648f
ce007c8
0abd490
2aab264
d2b8348
edc37a4
234121c
4aa0361
1e99cd0
b368c3e
40de949
6bc05a5
8a4f149
7be9b67
420a936
a1043e7
e5d8680
d2a114c
89112b9
18b0740
d7ba4ff
2060664
e93af30
adc398c
4e841ef
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 |
|---|---|---|
|
|
@@ -47,6 +47,7 @@ | |
| from models.settings.course_grading import CourseGradingModel | ||
| from cms.lib.xblock.runtime import handler_url, local_resource_url | ||
| from opaque_keys.edx.keys import UsageKey, CourseKey | ||
| from opaque_keys.edx.locator import LibraryUsageLocator | ||
|
|
||
| __all__ = ['orphan_handler', 'xblock_handler', 'xblock_view_handler', 'xblock_outline_handler'] | ||
|
|
||
|
|
@@ -650,7 +651,9 @@ def _get_module_info(xblock, rewrite_static_links=True): | |
| ) | ||
|
|
||
| # Pre-cache has changes for the entire course because we'll need it for the ancestor info | ||
| modulestore().has_changes(modulestore().get_course(xblock.location.course_key, depth=None)) | ||
| # Except library blocks which don't use draft/publish | ||
| if not isinstance(xblock.location, LibraryUsageLocator): | ||
| modulestore().has_changes(modulestore().get_courselike(xblock.location.course_key, depth=None)) | ||
|
|
||
| # Note that children aren't being returned until we have a use case. | ||
| return create_xblock_info(xblock, data=data, metadata=own_metadata(xblock), include_ancestor_info=True) | ||
|
|
@@ -691,12 +694,16 @@ def safe_get_username(user_id): | |
|
|
||
| return None | ||
|
|
||
| is_library_block = isinstance(xblock.location, LibraryUsageLocator) | ||
| is_xblock_unit = is_unit(xblock, parent_xblock) | ||
| # this should not be calculated for Sections and Subsections on Unit page | ||
| has_changes = modulestore().has_changes(xblock) if (is_xblock_unit or course_outline) else None | ||
| # this should not be calculated for Sections and Subsections on Unit page or for library blocks | ||
| has_changes = modulestore().has_changes(xblock) if (is_xblock_unit or course_outline) and not is_library_block else None | ||
|
|
||
| if graders is None: | ||
| graders = CourseGradingModel.fetch(xblock.location.course_key).graders | ||
| if xblock.category != "library": | ||
| graders = CourseGradingModel.fetch(xblock.location.course_key).graders | ||
|
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. @bradenmacdonald I have a subtle feeling here that it should be responsibility of
Member
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. @e-kolpakov Yes, |
||
| else: | ||
| graders = [] | ||
|
|
||
| # Compute the child info first so it can be included in aggregate information for the parent | ||
| should_visit_children = include_child_info and (course_outline and not is_xblock_unit or not course_outline) | ||
|
|
@@ -716,15 +723,15 @@ def safe_get_username(user_id): | |
| visibility_state = _compute_visibility_state(xblock, child_info, is_xblock_unit and has_changes) | ||
| else: | ||
| visibility_state = None | ||
| published = modulestore().has_published_version(xblock) | ||
| published = modulestore().has_published_version(xblock) if not is_library_block else None | ||
|
|
||
| xblock_info = { | ||
| "id": unicode(xblock.location), | ||
| "display_name": xblock.display_name_with_default, | ||
| "category": xblock.category, | ||
| "edited_on": get_default_time_display(xblock.subtree_edited_on) if xblock.subtree_edited_on else None, | ||
| "published": published, | ||
| "published_on": get_default_time_display(xblock.published_on) if xblock.published_on else None, | ||
| "published_on": get_default_time_display(xblock.published_on) if published and xblock.published_on else None, | ||
| "studio_url": xblock_studio_url(xblock, parent_xblock), | ||
| "released_to_students": datetime.now(UTC) > xblock.start, | ||
| "release_date": release_date, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,169 @@ | ||
| """ | ||
| Views related to content libraries. | ||
| A content library is a structure containing XBlocks which can be re-used in the | ||
| multiple courses. | ||
| """ | ||
| from __future__ import absolute_import | ||
|
|
||
| import json | ||
| import logging | ||
|
|
||
| from contentstore.views.item import create_xblock_info | ||
| from contentstore.utils import reverse_library_url | ||
| from django.http import HttpResponseBadRequest, HttpResponseNotAllowed, Http404 | ||
| from django.contrib.auth.decorators import login_required | ||
| from django.core.exceptions import PermissionDenied | ||
| from django.conf import settings | ||
| from django.utils.translation import ugettext as _ | ||
| from django_future.csrf import ensure_csrf_cookie | ||
| from edxmako.shortcuts import render_to_response | ||
| from opaque_keys import InvalidKeyError | ||
| from opaque_keys.edx.keys import CourseKey | ||
| from opaque_keys.edx.locator import LibraryLocator, LibraryUsageLocator | ||
| from xmodule.library_module import LibraryDescriptor | ||
| from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateCourseError | ||
| from xmodule.modulestore import ModuleStoreEnum | ||
| from xmodule.modulestore.django import modulestore | ||
|
|
||
| from .access import has_course_access | ||
| from .component import get_component_templates | ||
| from student.roles import CourseCreatorRole | ||
| from student import auth | ||
| from util.json_request import expect_json, JsonResponse | ||
|
|
||
| __all__ = ['library_handler'] | ||
|
|
||
| log = logging.getLogger(__name__) | ||
|
|
||
| LIBRARIES_ENABLED = settings.FEATURES.get('ENABLE_CONTENT_LIBRARIES', False) | ||
|
|
||
|
|
||
| @login_required | ||
| @ensure_csrf_cookie | ||
| def library_handler(request, library_key_string=None): | ||
| """ | ||
| RESTful interface to most content library related functionality. | ||
| """ | ||
| if not LIBRARIES_ENABLED: | ||
| raise Http404 # Should never happen because we test the feature in urls.py also | ||
|
|
||
| response_format = 'html' | ||
| if request.REQUEST.get('format', 'html') == 'json' or 'application/json' in request.META.get('HTTP_ACCEPT', 'text/html'): | ||
| response_format = 'json' | ||
|
|
||
| if library_key_string: | ||
| library_key = CourseKey.from_string(library_key_string) | ||
| if not isinstance(library_key, LibraryLocator): | ||
| raise Http404 # This is not a library | ||
| if not has_course_access(request.user, library_key): | ||
| raise PermissionDenied() | ||
|
|
||
| try: | ||
| library = modulestore().get_library(library_key) | ||
| if library is None: | ||
| raise ItemNotFoundError # Inconsistency: mixed modulestore returns None, whereas split raises exception | ||
| except ItemNotFoundError: | ||
| raise Http404 | ||
|
|
||
| if not isinstance(library, LibraryDescriptor): | ||
| return HttpResponseBadRequest("Key specified is not a library.") | ||
|
|
||
| if request.method == 'GET': | ||
| return library_blocks_view(request, library, response_format) | ||
| return HttpResponseNotAllowed(['GET']) | ||
|
|
||
| elif request.method == 'POST': | ||
| # Create a new library: | ||
| return _create_library(request) | ||
| elif request.method == 'GET': | ||
| # List all accessible libraries: | ||
| lib_info = [ | ||
| { | ||
| "display_name": lib.display_name, | ||
| "library_key": unicode(lib.location.library_key), | ||
| } | ||
| for lib in modulestore().get_libraries() | ||
| if has_course_access(request.user, lib.location.library_key) | ||
| ] | ||
| return JsonResponse(lib_info) | ||
| else: | ||
| return HttpResponseNotAllowed(['GET', 'POST']) | ||
|
|
||
|
|
||
| @expect_json | ||
| def _create_library(request): | ||
| """ | ||
| Helper method for creating a new library. | ||
| """ | ||
| if not auth.has_access(request.user, CourseCreatorRole()): | ||
| raise PermissionDenied() | ||
| try: | ||
| org = request.json['org'] | ||
| library = request.json.get('number', None) | ||
| if library is None: | ||
|
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. Equivalent to
Member
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. I don't think that's actually equivalent? I think the equivalent code would need to be: library = request.json.get('number', request.json.get('library', None))
if library is None:
raise KeyError... because I'm relying on the |
||
| library = request.json['library'] | ||
| display_name = request.json['display_name'] | ||
| store = modulestore() | ||
| with store.default_store(ModuleStoreEnum.Type.split): | ||
| new_lib = store.create_library( | ||
| org=org, | ||
| library=library, | ||
| user_id=request.user.id, | ||
| fields={"display_name": display_name}, | ||
| ) | ||
| except KeyError as error: | ||
| return JsonResponse({ | ||
| "ErrMsg": _("Unable to create library - missing expected JSON key '{err}'").format(err=error.message)} | ||
| ) | ||
| except InvalidKeyError as error: | ||
| return JsonResponse({ | ||
| "ErrMsg": _("Unable to create library - invalid data.\n\n{err}").format(name=display_name, err=error.message)} | ||
| ) | ||
| except DuplicateCourseError as error: | ||
| return JsonResponse({ | ||
| "ErrMsg": _("Unable to create library - one already exists with that key.\n\n{err}").format(err=error.message)} | ||
| ) | ||
|
|
||
| lib_key_str = unicode(new_lib.location.library_key) | ||
| return JsonResponse({ | ||
| 'url': reverse_library_url('library_handler', lib_key_str), | ||
| 'library_key': lib_key_str, | ||
| }) | ||
|
|
||
|
|
||
| def library_blocks_view(request, library, response_format): | ||
| """ | ||
| The main view of a course's content library. | ||
| Shows all the XBlocks in the library, and allows adding/editing/deleting | ||
| them. | ||
| Can be called with response_format="json" to get a JSON-formatted list of | ||
| the XBlocks in the library along with library metadata. | ||
| """ | ||
| children = library.children | ||
| if response_format == "json": | ||
| # The JSON response for this request is short and sweet: | ||
| prev_version = library.runtime.course_entry.structure['previous_version'] | ||
| return JsonResponse({ | ||
| "display_name": library.display_name, | ||
| "library_id": unicode(library.location.course_key), # library.course_id raises UndefinedContext - fix? | ||
| "version": unicode(library.runtime.course_entry.course_key.version), | ||
| "previous_version": unicode(prev_version) if prev_version else None, | ||
| "blocks": [unicode(x) for x in children], | ||
| }) | ||
|
|
||
| xblock_info = create_xblock_info(library, include_ancestor_info=False, graders=[]) | ||
|
|
||
| component_templates = get_component_templates(library) | ||
|
|
||
| assert isinstance(library.location.library_key, LibraryLocator) | ||
| assert isinstance(library.location, LibraryUsageLocator) | ||
|
|
||
| return render_to_response('library.html', { | ||
| 'context_library': library, | ||
| 'action': 'view', | ||
| 'xblock': library, | ||
| 'xblock_locator': library.location, | ||
| 'unit': None, | ||
| 'component_templates': json.dumps(component_templates), | ||
| 'xblock_info': xblock_info, | ||
| }) | ||
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.
Same code on line 655 - might make sense to extract method / function.