Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions common/djangoapps/xmodule_django/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def _strip_value(value, lookup='exact'):


class CourseKeyField(models.CharField):
description = "A SlashSeparatedCourseKey object, saved to the DB in the form of a string"
description = "A CourseKey object, saved to the DB in the form of a string"

__metaclass__ = models.SubfieldBase

Expand All @@ -84,7 +84,7 @@ def to_python(self, value):
return None

if isinstance(value, basestring):
return SlashSeparatedCourseKey.from_deprecated_string(value)
return CourseKey.from_string(value)
else:
return value

Expand Down
5 changes: 4 additions & 1 deletion common/lib/xmodule/setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@
"crowdsource_hinter = xmodule.crowdsource_hinter:CrowdsourceHinterDescriptor",
"lti = xmodule.lti_module:LTIDescriptor",
]
XBLOCKS = [
"library = xmodule.library_root_xblock:LibraryRoot",
]

setup(
name="XModule",
Expand All @@ -64,7 +67,7 @@
# See http://guide.python-distribute.org/creation.html#entry-points
# for a description of entry_points
entry_points={
'xblock.v1': XMODULES,
'xblock.v1': XMODULES + XBLOCKS,
'xmodule.v1': XMODULES,
'console_scripts': [
'xmodule_assets = xmodule.static_content:main',
Expand Down
92 changes: 92 additions & 0 deletions common/lib/xmodule/xmodule/library_root_xblock.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
"""
'library' XBlock (LibraryRoot)
"""
import logging

from .studio_editable import StudioEditableModule
from xblock.core import XBlock
from xblock.fields import Scope, String, List
from xblock.fragment import Fragment

log = logging.getLogger(__name__)

# Make '_' a no-op so we can scrape strings
_ = lambda text: text


class LibraryRoot(XBlock):
"""
The LibraryRoot is the root XBlock of a content library. All other blocks in
the library are its children. It contains metadata such as the library's
display_name.
"""
display_name = String(
help=_("Enter the name of the library as it should appear in Studio."),
default="Library",
display_name=_("Library Display Name"),
scope=Scope.settings
)
advanced_modules = List(
display_name=_("Advanced Module List"),
help=_("Enter the names of the advanced components to use in your library."),
scope=Scope.settings
)
has_children = True
has_author_view = True

def __unicode__(self):
return u"Library: {}".format(self.display_name)

def __str__(self):
return unicode(self).encode('utf-8')

def author_view(self, context):
"""
Renders the Studio preview view, which supports drag and drop.
"""
fragment = Fragment()
contents = []

for child_key in self.children: # pylint: disable=E1101
context['reorderable_items'].add(child_key)
child = self.runtime.get_block(child_key)
rendered_child = self.runtime.render_child(child, StudioEditableModule.get_preview_view_name(child), context)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's too bad that you have to do StudioEditableModule.get_preview_view_name, rather than just having a default author_view that simply calls the student_view, so that Studio could just always render author_view.

@andy-armstrong @cahrens: Future enhancement?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah, I was struggling with how to "mix in" special Studio editing capabilities without overriding custom xblock implementations. I'd love us to reconcile the various semi-documented view methods (student_view, author_view, studio_view, new view TBD for default editing support of xblocks).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@cpennington I believe this note is not actionable now, at least on my side. Am I right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, that's right.

fragment.add_frag_resources(rendered_child)

contents.append({
'id': unicode(child_key),
'content': rendered_child.content,
})

fragment.add_content(self.runtime.render_template("studio_render_children_view.html", {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As an XBlock, this really shouldn't be using self.runtime.render_template (that's a holdover from xmodule), and should instead be doing its own templating.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@cpennington this method will be changed in follow up PRs, so I would suggest leaving it as is for now, as it works and to be changed soon.

If you are convinced it should be done in scope of this PR, what's the recommended way for rendering templates in XBlocks than? I believe there're no other XBlocks in edx-platform repo (except for pending review Discussion XBlock), so some hints on what's allowed and what's not would be appreciated :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you're going to be working on it soon, then it's fine to leave as-is for now.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In general, lean towards not using anything out of self.runtime that's not accessible via a self.runtime.service(..) call, as those are likely to be attributes that we're going to be getting rid of.

For rendering in particular, you can just use the Mako library directly, rather than using it via self.runtime.render_template.

'items': contents,
'xblock_context': context,
'can_add': True,
'can_reorder': True,
}))
return fragment

@property
def display_org_with_default(self):
"""
Org display names are not implemented. This just provides API compatibility with CourseDescriptor.
Always returns the raw 'org' field from the key.
"""
return self.scope_ids.usage_id.course_key.org

@property
def display_number_with_default(self):
"""
Display numbers are not implemented. This just provides API compatibility with CourseDescriptor.
Always returns the raw 'library' field from the key.
"""
return self.scope_ids.usage_id.course_key.library

@classmethod
def parse_xml(cls, xml_data, system, id_generator, **kwargs):
""" XML support not yet implemented. """
raise NotImplementedError

def add_xml_to_node(self, resource_fs):
""" XML support not yet implemented. """
raise NotImplementedError
1 change: 1 addition & 0 deletions common/lib/xmodule/xmodule/modulestore/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ class BranchName(object):
"""
draft = 'draft-branch'
published = 'published-branch'
library = 'library'

class UserID(object):
"""
Expand Down
65 changes: 65 additions & 0 deletions common/lib/xmodule/xmodule/modulestore/mixed.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

from opaque_keys import InvalidKeyError
from opaque_keys.edx.keys import CourseKey, AssetKey
from opaque_keys.edx.locator import LibraryLocator
from opaque_keys.edx.locations import SlashSeparatedCourseKey
from xmodule.assetstore import AssetMetadata

Expand All @@ -25,6 +26,7 @@
new_contract('CourseKey', CourseKey)
new_contract('AssetKey', AssetKey)
new_contract('AssetMetadata', AssetMetadata)
new_contract('LibraryLocator', LibraryLocator)

log = logging.getLogger(__name__)

Expand Down Expand Up @@ -259,6 +261,23 @@ def get_courses(self, **kwargs):
courses[course_id] = course
return courses.values()

@strip_key
def get_libraries(self, **kwargs):
"""
Returns a list containing the top level XBlock of the libraries (LibraryRoot) in this modulestore.
"""
libraries = {}
for store in self.modulestores:
if not hasattr(store, 'get_libraries'):
continue
# filter out ones which were fetched from earlier stores but locations may not be ==
for course in store.get_libraries(**kwargs):
course_id = self._clean_course_id_for_mapping(course.location)
if course_id not in libraries:
# course is indeed unique. save it in result
libraries[course_id] = course
return libraries.values()

def make_course_key(self, org, course, run):
"""
Return a valid :class:`~opaque_keys.edx.keys.CourseKey` for this modulestore
Expand Down Expand Up @@ -290,6 +309,24 @@ def get_course(self, course_key, depth=0, **kwargs):
except ItemNotFoundError:
return None

@strip_key
@contract(library_key='LibraryLocator')
def get_library(self, library_key, depth=0, **kwargs):
"""
returns the library block associated with the given key. If no such library exists,
it returns None

:param library_key: must be a LibraryLocator
"""
try:
store = self._verify_modulestore_support(library_key, 'get_library')
return store.get_library(library_key, depth=depth, **kwargs)
except NotImplementedError:
log.exception("Modulestore configured for %s does not have get_library method", library_key)
return None
except ItemNotFoundError:
return None

@strip_key
def has_course(self, course_id, ignore_case=False, **kwargs):
"""
Expand Down Expand Up @@ -507,6 +544,34 @@ def create_course(self, org, course, run, user_id, **kwargs):

return course

@strip_key
def create_library(self, org, library, user_id, fields, **kwargs):
"""
Creates and returns a new library.

Args:
org (str): the organization that owns the course
library (str): the code/number/name of the library
user_id: id of the user creating the course
fields (dict): Fields to set on the course at initialization - e.g. display_name
kwargs: Any optional arguments understood by a subset of modulestores to customize instantiation

Returns: a LibraryRoot
"""
# first make sure an existing course/lib doesn't already exist in the mapping
lib_key = LibraryLocator(org=org, library=library)
if lib_key in self.mappings:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps do a case-insensitive check to ensure we don't have libraries whose only distinction is case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@dmitchell I believe this shouldn't be done as the proper way to do this is to add overloaded __hash__ and __eq__ to LibraryLocator which is in opaque_keys repository. So it cant happen in this PR anyway :)

One more consideration - CourseLocator exhibit the same behavior, so to keep courses and libraries consistent it's better modify both of them, or none.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's a difference between saying a locator is case-insensitive and saying that we don't allow the user to create something whose case insensitive id == another thing's. We are, however, discussing whether all locators should be case-insensitive.

raise DuplicateCourseError(lib_key, lib_key)

# create the library
store = self._verify_modulestore_support(None, 'create_library')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmmm, this leads me to wonder whether we should change the definition of _verify_modulestore_support to special case None as "return the default_store or the first store which supports the given method". Then, your callers wouldn't need to wrap with default_store and it would basically mean that None as a course key means "Find where I can do this"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@dmitchell it makes some sense, but I would suggest creating separate method for that, rather than adding another mode to _verify_modulestore_support. It would also make a terrible naming: verify checks for something, not finds.

So, my suggestion is that it's possible to do so, but it would better be a separate method (e.g. _find_modulestore_with_method or something). And since it is not directly required by anything in this PR, it should be done separately.

Are you ok with that?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@dmitchell just to clarify it - as of now, we're not implementing this as part of Content Libraries effort. Please let me or @antoviaque know if it's required/needed so that we schedule this task for implementation in (near?) future.

library = store.create_library(org, library, user_id, fields, **kwargs)

# add new library to the mapping
self.mappings[lib_key] = store

return library

@strip_key
def clone_course(self, source_course_id, dest_course_id, user_id, fields=None, **kwargs):
"""
Expand Down
4 changes: 3 additions & 1 deletion common/lib/xmodule/xmodule/modulestore/mongo/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
from opaque_keys.edx.keys import UsageKey, CourseKey, AssetKey
from opaque_keys.edx.locations import Location
from opaque_keys.edx.locations import SlashSeparatedCourseKey
from opaque_keys.edx.locator import CourseLocator
from opaque_keys.edx.locator import CourseLocator, LibraryLocator

from xblock.core import XBlock
from xblock.exceptions import InvalidScopeError
Expand Down Expand Up @@ -875,6 +875,8 @@ def has_course(self, course_key, ignore_case=False, **kwargs):
otherwise, do a case sensitive search
"""
assert(isinstance(course_key, CourseKey))
if isinstance(course_key, LibraryLocator):
return None # Libraries require split mongo
course_key = self.fill_in_run(course_key)
location = course_key.make_usage_key('course', course_key.run)
if ignore_case:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from lazy import lazy
from xblock.runtime import KvsFieldData
from xblock.fields import ScopeIds
from opaque_keys.edx.locator import BlockUsageLocator, LocalId, CourseLocator, DefinitionLocator
from opaque_keys.edx.locator import BlockUsageLocator, LocalId, CourseLocator, LibraryLocator, DefinitionLocator
from xmodule.mako_module import MakoDescriptorSystem
from xmodule.error_module import ErrorDescriptor
from xmodule.errortracker import exc_info_to_str
Expand All @@ -19,6 +19,8 @@
log = logging.getLogger(__name__)

new_contract('BlockUsageLocator', BlockUsageLocator)
new_contract('CourseLocator', CourseLocator)
new_contract('LibraryLocator', LibraryLocator)
new_contract('BlockKey', BlockKey)
new_contract('CourseEnvelope', CourseEnvelope)

Expand Down Expand Up @@ -115,7 +117,7 @@ def _load_item(self, usage_key, course_entry_override=None, **kwargs):
self.modulestore.cache_block(course_key, version_guid, block_key, block)
return block

@contract(block_key=BlockKey, course_key=CourseLocator)
@contract(block_key=BlockKey, course_key="CourseLocator | LibraryLocator")
def get_module_data(self, block_key, course_key):
"""
Get block from module_data adding it to module_data if it's not already there but is in the structure
Expand Down Expand Up @@ -178,8 +180,8 @@ def xblock_from_json(
if definition_id is None:
definition_id = LocalId()

block_locator = BlockUsageLocator(
course_key,
# Construct the Block Usage Locator:
block_locator = course_key.make_usage_key(
block_type=block_key.type,
block_id=block_key.id,
)
Expand Down
Loading