Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
6f11b98
initial commit for a mixed module store which can interoperate with b…
Jul 22, 2013
b5253b5
add a set_modulestore_configuration to the modulestore interface. Thi…
Jul 24, 2013
8298897
WIP: added a get_modulestore_type. Added some unit tests. changed ove…
Jul 24, 2013
2616d8f
remove unneeded type check on the modulestore
Jul 24, 2013
cff93d3
WIP
Jul 25, 2013
5298f54
fix tests
Jul 25, 2013
fa61bdf
remove debugging logging
Jul 25, 2013
54bd317
remove another debug logging message
Jul 25, 2013
9f14f1e
update file comment on MixedModuleStore
Jul 25, 2013
aa8b054
fix missed conflict resolution
Jul 25, 2013
e4eea6c
some additional comments to clarify the partially bogus course_id.
Jul 30, 2013
52928d1
update SplitModuleStore tests to pull the course_id from the locator
Jul 30, 2013
7bdc4c5
fix errant conflict resolution
Aug 5, 2013
7a80a9c
fix broken test after rebase
Aug 8, 2013
9cc796d
fix one more 'revision' -> 'branch' argument name change
Aug 8, 2013
5ee5bea
fix bad merge conflict resolution
Aug 13, 2013
0b8866e
forgot to return dictionary on get_errored_courses
Aug 13, 2013
6121916
change over the modulestore configuration to be a dict and use a prop…
Aug 13, 2013
c6cde6a
resolve incorrect merge conflict resolution
Aug 14, 2013
ed584a9
fix pep8 violations
Aug 15, 2013
cf715cb
fix pylint violations
Aug 15, 2013
bca2018
add unit tests for all methods in MixedModuleStore
Aug 16, 2013
ae6f97a
add test for test_static_url_generation_from_courseid
Aug 16, 2013
13ff461
add a filter to get_courses to not surface any courses that haven't b…
Aug 16, 2013
d1ce55f
have the test teardown explicitly call destroy_db
Aug 16, 2013
1128f36
add changelog entry
Aug 16, 2013
bd71a2c
add unit test for video_caption asset path
Aug 16, 2013
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
2 changes: 2 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ LMS: Removed press releases

Common: Updated Sass and Bourbon libraries, added Neat library

LMS: Add a MixedModuleStore to aggregate the XMLModuleStore and MongoMonduleStore

LMS: Users are no longer auto-activated if they click "reset password"
This is now done when they click on the link in the reset password
email they receive (along with usual path through activation email).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@

from request_cache.middleware import RequestCache

from django.core.cache import get_cache

CACHE = get_cache('mongo_metadata_inheritance')

class Command(BaseCommand):
help = '''Enumerates through the course and find common errors'''
Expand All @@ -19,7 +22,10 @@ def handle(self, *args, **options):
store = modulestore()

# setup a request cache so we don't throttle the DB with all the metadata inheritance requests
store.request_cache = RequestCache.get_request_cache()
store.set_modulestore_configuration({
'metadata_inheritance_cache_subsystem': CACHE,
'request_cache': RequestCache.get_request_cache()
})

course = store.get_item(loc, depth=3)

Expand Down
11 changes: 5 additions & 6 deletions cms/djangoapps/contentstore/management/commands/clone_course.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,6 @@
from request_cache.middleware import RequestCache
from django.core.cache import get_cache

#
# To run from command line: rake cms:delete_course LOC=MITx/111/Foo1
#

CACHE = get_cache('mongo_metadata_inheritance')

class Command(BaseCommand):
Expand All @@ -36,8 +32,11 @@ def handle(self, *args, **options):
mstore = modulestore('direct')
cstore = contentstore()

mstore.metadata_inheritance_cache_subsystem = CACHE
mstore.request_cache = RequestCache.get_request_cache()
mstore.set_modulestore_configuration({
'metadata_inheritance_cache_subsystem': CACHE,
'request_cache': RequestCache.get_request_cache()
})

org, course_num, run = dest_course_id.split("/")
mstore.ignore_write_events_on_courses.append('{0}/{1}'.format(org, course_num))

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,11 @@ def handle(self, *args, **options):
ms = modulestore('direct')
cs = contentstore()

ms.metadata_inheritance_cache_subsystem = CACHE
ms.request_cache = RequestCache.get_request_cache()
ms.set_modulestore_configuration({
'metadata_inheritance_cache_subsystem': CACHE,
'request_cache': RequestCache.get_request_cache()
})

org, course_num, run = course_id.split("/")
ms.ignore_write_events_on_courses.append('{0}/{1}'.format(org, course_num))

Expand Down
11 changes: 3 additions & 8 deletions cms/djangoapps/contentstore/module_info_model.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
from static_replace import replace_static_urls
from xmodule.modulestore.exceptions import ItemNotFoundError
from xmodule.modulestore import Location


def get_module_info(store, location, rewrite_static_links=False):
Expand All @@ -13,16 +12,12 @@ def get_module_info(store, location, rewrite_static_links=False):

data = module.data
if rewrite_static_links:
# we pass a partially bogus course_id as we don't have the RUN information passed yet
# through the CMS. Also the contentstore is also not RUN-aware at this point in time.
data = replace_static_urls(
module.data,
None,
course_namespace=Location([
module.location.tag,
module.location.org,
module.location.course,
None,
None
])
course_id=module.location.org + '/' + module.location.course + '/BOGUS_RUN_REPLACE_WHEN_AVAILABLE'
)

return {
Expand Down
14 changes: 14 additions & 0 deletions cms/djangoapps/contentstore/tests/test_contentstore.py
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,20 @@ def test_module_preview_in_whitelist(self):
resp = self.client.get(url)
self.assertEqual(resp.status_code, 200)

def test_video_module_caption_asset_path(self):
'''
This verifies that a video caption url is as we expect it to be
'''
direct_store = modulestore('direct')
import_from_xml(direct_store, 'common/test/data/', ['toy'])

# also try a custom response which will trigger the 'is this course in whitelist' logic
video_module_location = Location(['i4x', 'edX', 'toy', 'video', 'sample_video', None])
url = reverse('preview_component', kwargs={'location': video_module_location.url()})
resp = self.client.get(url)
self.assertEqual(resp.status_code, 200)
self.assertContains(resp, 'data-caption-asset-path="/c4x/edX/toy/asset/subs_"')

def test_delete(self):
direct_store = modulestore('direct')
CourseFactory.create(org='edX', course='999', display_name='Robot Super Course')
Expand Down
6 changes: 4 additions & 2 deletions cms/djangoapps/contentstore/views/preview.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ def preview_model_data(descriptor):
get_module=partial(load_preview_module, request, preview_id),
render_template=render_from_lms,
debug=True,
replace_urls=partial(static_replace.replace_static_urls, data_directory=None, course_namespace=descriptor.location),
replace_urls=partial(static_replace.replace_static_urls, data_directory=None, course_id=course_id),
user=request.user,
xblock_model_data=preview_model_data,
can_execute_unsafe_code=(lambda: can_execute_unsafe_code(course_id)),
Expand Down Expand Up @@ -155,10 +155,12 @@ def load_preview_module(request, preview_id, descriptor):
"xmodule_display.html",
)

# we pass a partially bogus course_id as we don't have the RUN information passed yet
# through the CMS. Also the contentstore is also not RUN-aware at this point in time.
module.get_html = replace_static_urls(
module.get_html,
getattr(module, 'data_dir', module.location.course),
course_namespace=Location([module.location.tag, module.location.org, module.location.course, None, None])
course_id=module.location.org + '/' + module.location.course + '/BOGUS_RUN_REPLACE_WHEN_AVAILABLE'
)

module.get_html = save_module(
Expand Down
7 changes: 5 additions & 2 deletions cms/one_time_startup.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@
CACHE = get_cache('mongo_metadata_inheritance')
for store_name in settings.MODULESTORE:
store = modulestore(store_name)
store.metadata_inheritance_cache_subsystem = CACHE
store.request_cache = RequestCache.get_request_cache()

store.set_modulestore_configuration({
'metadata_inheritance_cache_subsystem': CACHE,
'request_cache': RequestCache.get_request_cache()
})

modulestore_update_signal = Signal(providing_args=['modulestore', 'course_id', 'location'])
store.modulestore_update_signal = modulestore_update_signal
Expand Down
10 changes: 5 additions & 5 deletions common/djangoapps/static_replace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from django.conf import settings

from xmodule.modulestore.django import modulestore
from xmodule.modulestore.xml import XMLModuleStore
from xmodule.modulestore import XML_MODULESTORE_TYPE
from xmodule.contentstore.content import StaticContent

log = logging.getLogger(__name__)
Expand Down Expand Up @@ -90,7 +90,7 @@ def replace_course_url(match):
return re.sub(_url_replace_regex('/course/'), replace_course_url, text)


def replace_static_urls(text, data_directory, course_namespace=None):
def replace_static_urls(text, data_directory, course_id=None):
"""
Replace /static/$stuff urls either with their correct url as generated by collectstatic,
(/static/$md5_hashed_stuff) or by the course-specific content static url
Expand All @@ -99,7 +99,7 @@ def replace_static_urls(text, data_directory, course_namespace=None):

text: The source text to do the substitution in
data_directory: The directory in which course data is stored
course_namespace: The course identifier used to distinguish static content for this course in studio
course_id: The course identifier used to distinguish static content for this course in studio
"""

def replace_static_url(match):
Expand All @@ -116,15 +116,15 @@ def replace_static_url(match):
if settings.DEBUG and finders.find(rest, True):
return original
# if we're running with a MongoBacked store course_namespace is not None, then use studio style urls
elif course_namespace is not None and not isinstance(modulestore(), XMLModuleStore):
elif course_id and modulestore().get_modulestore_type(course_id) != XML_MODULESTORE_TYPE:
# first look in the static file pipeline and see if we are trying to reference
# a piece of static content which is in the mitx repo (e.g. JS associated with an xmodule)
if staticfiles_storage.exists(rest):
url = staticfiles_storage.url(rest)
else:
# if not, then assume it's courseware specific content and then look in the
# Mongo-backed database
url = StaticContent.convert_legacy_static_url(rest, course_namespace)
url = StaticContent.convert_legacy_static_url_with_course_id(rest, course_id)
# Otherwise, look the file up in staticfiles_storage, and append the data directory if needed
else:
course_path = "/".join((data_directory, rest))
Expand Down
9 changes: 4 additions & 5 deletions common/djangoapps/static_replace/test/test_static_replace.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

DATA_DIRECTORY = 'data_dir'
COURSE_ID = 'org/course/run'
NAMESPACE = Location('org', 'course', 'run', None, None)
STATIC_SOURCE = '"/static/file.png"'


Expand Down Expand Up @@ -52,18 +51,18 @@ def test_storage_url_not_exists(mock_storage):
def test_mongo_filestore(mock_modulestore, mock_static_content):

mock_modulestore.return_value = Mock(MongoModuleStore)
mock_static_content.convert_legacy_static_url.return_value = "c4x://mock_url"
mock_static_content.convert_legacy_static_url_with_course_id.return_value = "c4x://mock_url"

# No namespace => no change to path
assert_equals('"/static/data_dir/file.png"', replace_static_urls(STATIC_SOURCE, DATA_DIRECTORY))

# Namespace => content url
assert_equals(
'"' + mock_static_content.convert_legacy_static_url.return_value + '"',
replace_static_urls(STATIC_SOURCE, DATA_DIRECTORY, NAMESPACE)
'"' + mock_static_content.convert_legacy_static_url_with_course_id.return_value + '"',

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.

Seems like it would be better to write this line as '"{}"'.format(mock_static_content.convert_legacy_static_url_with_course_id())

replace_static_urls(STATIC_SOURCE, DATA_DIRECTORY, course_id=COURSE_ID)
)

mock_static_content.convert_legacy_static_url.assert_called_once_with('file.png', NAMESPACE)
mock_static_content.convert_legacy_static_url_with_course_id.assert_called_once_with('file.png', COURSE_ID)


@patch('static_replace.settings')
Expand Down
1 change: 1 addition & 0 deletions common/djangoapps/student/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ def index(request, extra_context={}, user=None):
courses = sort_by_announcement(courses)

context = {'courses': courses}

context.update(extra_context)
return render_to_response('index.html', context)

Expand Down
4 changes: 2 additions & 2 deletions common/djangoapps/xmodule_modifiers.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ def _get_html():
return _get_html


def replace_static_urls(get_html, data_dir, course_namespace=None):
def replace_static_urls(get_html, data_dir, course_id=None):
"""
Updates the supplied module with a new get_html function that wraps
the old get_html function and substitutes urls of the form /static/...
Expand All @@ -85,7 +85,7 @@ def replace_static_urls(get_html, data_dir, course_namespace=None):

@wraps(get_html)
def _get_html():
return static_replace.replace_static_urls(get_html(), data_dir, course_namespace)
return static_replace.replace_static_urls(get_html(), data_dir, course_id)
return _get_html


Expand Down
10 changes: 10 additions & 0 deletions common/lib/xmodule/xmodule/contentstore/content.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,16 @@ def convert_legacy_static_url(path, course_namespace):
loc = StaticContent.compute_location(course_namespace.org, course_namespace.course, path)
return StaticContent.get_url_path_from_location(loc)

@staticmethod
def convert_legacy_static_url_with_course_id(path, course_id):

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.

Pls unit test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

"""
Returns a path to a piece of static content when we are provided with a filepath and
a course_id
"""
org, course_num, __ = course_id.split("/")
loc = StaticContent.compute_location(org, course_num, path)
return StaticContent.get_url_path_from_location(loc)

def stream_data(self):
yield self._data

Expand Down
41 changes: 39 additions & 2 deletions common/lib/xmodule/xmodule/modulestore/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

log = logging.getLogger('mitx.' + 'modulestore')

MONGO_MODULESTORE_TYPE = 'mongo'
XML_MODULESTORE_TYPE = 'xml'

URL_RE = re.compile("""
(?P<tag>[^:]+)://?
Expand Down Expand Up @@ -258,7 +260,7 @@ class ModuleStore(object):
An abstract interface for a database backend that stores XModuleDescriptor
instances
"""
def has_item(self, location):
def has_item(self, course_id, location):
"""
Returns True if location exists in this ModuleStore.
"""
Expand Down Expand Up @@ -384,6 +386,20 @@ def get_errored_courses(self):
"""
raise NotImplementedError

def set_modulestore_configuration(self, config_dict):

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.

Pythonically, this would probably just be written as a @property.setter method in the MixedModuleStore, and everywhere else the modulestore configuration would just be accessed as a regular attribute (xmlstore.configuration = {}).

I don't think this should block the PR, but it would be nice to capture it as a cleanup task (or, if someone has time, to clean it up now).

'''
Allows for runtime configuration of the modulestore. In particular this is how the
application (LMS/CMS) can pass down Django related configuration information, e.g. caches, etc.
'''
raise NotImplementedError

def get_modulestore_type(self, course_id):
"""
Returns a type which identifies which modulestore is servicing the given
course_id. The return can be either "xml" (for XML based courses) or "mongo" for MongoDB backed courses
"""
raise NotImplementedError


class ModuleStoreBase(ModuleStore):
'''
Expand All @@ -394,7 +410,7 @@ def __init__(self):
Set up the error-tracking logic.
'''
self._location_errors = {} # location -> ErrorLog
self.metadata_inheritance_cache = None
self.modulestore_configuration = {}
self.modulestore_update_signal = None # can be set by runtime to route notifications of datastore changes

def _get_errorlog(self, location):
Expand Down Expand Up @@ -439,6 +455,27 @@ def get_course(self, course_id):
return c
return None

@property
def metadata_inheritance_cache_subsystem(self):
"""
Exposes an accessor to the runtime configuration for the metadata inheritance cache
"""
return self.modulestore_configuration.get('metadata_inheritance_cache_subsystem', None)

@property
def request_cache(self):
"""
Exposes an accessor to the runtime configuration for the request cache
"""
return self.modulestore_configuration.get('request_cache', None)

def set_modulestore_configuration(self, config_dict):
"""
This is the base implementation of the interface, all we need to do is store
two possible configurations as attributes on the class
"""
self.modulestore_configuration = config_dict

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.

Is this method even necessary?



def namedtuple_to_son(namedtuple, prefix=''):
"""
Expand Down
39 changes: 23 additions & 16 deletions common/lib/xmodule/xmodule/modulestore/django.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,24 +25,31 @@ def load_function(path):
return getattr(import_module(module_path), name)


def modulestore(name='default'):
if name not in _MODULESTORES:
class_ = load_function(settings.MODULESTORE[name]['ENGINE'])
def create_modulestore_instance(engine, options):

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.

This method isn't covered by unit tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

"""
This will return a new instance of a modulestore given an engine and options
"""
class_ = load_function(engine)

options = {}
_options = {}
_options.update(options)

options.update(settings.MODULESTORE[name]['OPTIONS'])
for key in FUNCTION_KEYS:
if key in options:
options[key] = load_function(options[key])
for key in FUNCTION_KEYS:
if key in _options and isinstance(_options[key], basestring):
_options[key] = load_function(_options[key])

_MODULESTORES[name] = class_(
**options
)
return class_(
**_options
)

return _MODULESTORES[name]

# if 'DJANGO_SETTINGS_MODULE' in environ:
# # Initialize the modulestores immediately
# for store_name in settings.MODULESTORE:
# modulestore(store_name)
def modulestore(name='default'):
"""
This returns an instance of a modulestore of given name. This will wither return an existing
modulestore or create a new one
"""
if name not in _MODULESTORES:
_MODULESTORES[name] = create_modulestore_instance(settings.MODULESTORE[name]['ENGINE'],
settings.MODULESTORE[name]['OPTIONS'])

return _MODULESTORES[name]
Loading