Commit 594a73d4 by chrisndodge

Merge pull request #722 from edx/feature/cdodge/add-a-mixed-modulestore

Feature/cdodge/add a mixed modulestore
parents 7f126f13 bd71a2ce
......@@ -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).
......
......@@ -5,6 +5,9 @@ from xmodule.course_module import CourseDescriptor
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'''
......@@ -19,7 +22,10 @@ class Command(BaseCommand):
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)
......
......@@ -15,10 +15,6 @@ from auth.authz import _copy_course_group
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):
......@@ -36,8 +32,11 @@ class Command(BaseCommand):
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))
......
......@@ -36,8 +36,11 @@ class Command(BaseCommand):
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))
......
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):
......@@ -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 {
......
......@@ -400,6 +400,20 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
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')
......
......@@ -116,7 +116,7 @@ def preview_module_system(request, preview_id, 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)),
......@@ -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(
......
......@@ -9,8 +9,11 @@ from django.core.cache import get_cache
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
......
......@@ -6,7 +6,7 @@ from staticfiles import finders
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__)
......@@ -90,7 +90,7 @@ def replace_course_urls(text, course_id):
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
......@@ -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):
......@@ -116,7 +116,7 @@ def replace_static_urls(text, data_directory, course_namespace=None):
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):
......@@ -124,7 +124,7 @@ def replace_static_urls(text, data_directory, course_namespace=None):
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))
......
......@@ -10,7 +10,6 @@ from xmodule.modulestore.xml import XMLModuleStore
DATA_DIRECTORY = 'data_dir'
COURSE_ID = 'org/course/run'
NAMESPACE = Location('org', 'course', 'run', None, None)
STATIC_SOURCE = '"/static/file.png"'
......@@ -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 + '"',
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')
......
......@@ -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)
......
......@@ -76,7 +76,7 @@ def replace_course_urls(get_html, course_id):
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/...
......@@ -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
......
......@@ -100,6 +100,16 @@ class StaticContent(object):
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):
"""
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
......
......@@ -14,6 +14,8 @@ from bson.son import SON
log = logging.getLogger('mitx.' + 'modulestore')
MONGO_MODULESTORE_TYPE = 'mongo'
XML_MODULESTORE_TYPE = 'xml'
URL_RE = re.compile("""
(?P<tag>[^:]+)://?
......@@ -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.
"""
......@@ -384,6 +386,20 @@ class ModuleStore(object):
"""
raise NotImplementedError
def set_modulestore_configuration(self, config_dict):
'''
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):
'''
......@@ -394,7 +410,7 @@ class ModuleStoreBase(ModuleStore):
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):
......@@ -439,6 +455,27 @@ class ModuleStoreBase(ModuleStore):
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
def namedtuple_to_son(namedtuple, prefix=''):
"""
......
......@@ -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):
"""
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]
"""
MixedModuleStore allows for aggregation between multiple modulestores.
In this way, courses can be served up both - say - XMLModuleStore or MongoModuleStore
IMPORTANT: This modulestore only supports READONLY applications, e.g. LMS
"""
from . import ModuleStoreBase
from django import create_modulestore_instance
import logging
log = logging.getLogger(__name__)
class MixedModuleStore(ModuleStoreBase):
"""
ModuleStore that can be backed by either XML or Mongo
"""
def __init__(self, mappings, stores):
"""
Initialize a MixedModuleStore. Here we look into our passed in kwargs which should be a
collection of other modulestore configuration informations
"""
super(MixedModuleStore, self).__init__()
self.modulestores = {}
self.mappings = mappings
if 'default' not in stores:
raise Exception('Missing a default modulestore in the MixedModuleStore __init__ method.')
for key in stores:
self.modulestores[key] = create_modulestore_instance(stores[key]['ENGINE'],
stores[key]['OPTIONS'])
def _get_modulestore_for_courseid(self, course_id):
"""
For a given course_id, look in the mapping table and see if it has been pinned
to a particular modulestore
"""
mapping = self.mappings.get(course_id, 'default')
return self.modulestores[mapping]
def has_item(self, course_id, location):
return self._get_modulestore_for_courseid(course_id).has_item(course_id, location)
def get_item(self, location, depth=0):
"""
This method is explicitly not implemented as we need a course_id to disambiguate
We should be able to fix this when the data-model rearchitecting is done
"""
raise NotImplementedError
def get_instance(self, course_id, location, depth=0):
return self._get_modulestore_for_courseid(course_id).get_instance(course_id, location, depth)
def get_items(self, location, course_id=None, depth=0):
"""
Returns a list of XModuleDescriptor instances for the items
that match location. Any element of location that is None is treated
as a wildcard that matches any value
location: Something that can be passed to Location
depth: An argument that some module stores may use to prefetch
descendents of the queried modules for more efficient results later
in the request. The depth is counted in the number of calls to
get_children() to cache. None indicates to cache all descendents
"""
if not course_id:
raise Exception("Must pass in a course_id when calling get_items() with MixedModuleStore")
return self._get_modulestore_for_courseid(course_id).get_items(location, course_id, depth)
def update_item(self, location, data, allow_not_found=False):
"""
MixedModuleStore is for read-only (aka LMS)
"""
raise NotImplementedError
def update_children(self, location, children):
"""
MixedModuleStore is for read-only (aka LMS)
"""
raise NotImplementedError
def update_metadata(self, location, metadata):
"""
MixedModuleStore is for read-only (aka LMS)
"""
raise NotImplementedError
def delete_item(self, location):
"""
MixedModuleStore is for read-only (aka LMS)
"""
raise NotImplementedError
def get_courses(self):
'''
Returns a list containing the top level XModuleDescriptors of the courses
in this modulestore.
'''
courses = []
for key in self.modulestores:
store_courses = self.modulestores[key].get_courses()
# If the store has not been labeled as 'default' then we should
# only surface courses that have a mapping entry, for example the XMLModuleStore will
# slurp up anything that is on disk, however, we don't want to surface those to
# consumers *unless* there is an explicit mapping in the configuration
if key != 'default':
for course in store_courses:
# make sure that the courseId is mapped to the store in question
if key == self.mappings.get(course.location.course_id, 'default'):
courses = courses + ([course])
else:
# if we're the 'default' store provider, then we surface all courses hosted in
# that store provider
courses = courses + (store_courses)
return courses
def get_course(self, course_id):
"""
returns the course module associated with the course_id
"""
return self._get_modulestore_for_courseid(course_id).get_course(course_id)
def get_parent_locations(self, location, course_id):
"""
returns the parent locations for a given lcoation and course_id
"""
return self._get_modulestore_for_courseid(course_id).get_parent_locations(location, course_id)
def set_modulestore_configuration(self, config_dict):
"""
This implementation of the interface method will pass along the configuration to all ModuleStore
instances
"""
for store in self.modulestores.values():
store.set_modulestore_configuration(config_dict)
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
"""
return self._get_modulestore_for_courseid(course_id).get_modulestore_type(course_id)
def get_errored_courses(self):
"""
Return a dictionary of course_dir -> [(msg, exception_str)], for each
course_dir where course loading failed.
"""
errs = {}
for store in self.modulestores.values():
errs.update(store.get_errored_courses())
return errs
......@@ -32,7 +32,7 @@ from xmodule.error_module import ErrorDescriptor
from xblock.runtime import DbModel, KeyValueStore, InvalidScopeError
from xblock.core import Scope
from xmodule.modulestore import ModuleStoreBase, Location, namedtuple_to_son
from xmodule.modulestore import ModuleStoreBase, Location, namedtuple_to_son, MONGO_MODULESTORE_TYPE
from xmodule.modulestore.exceptions import ItemNotFoundError
from xmodule.modulestore.inheritance import own_metadata, INHERITABLE_METADATA, inherit_metadata
......@@ -270,8 +270,7 @@ class MongoModuleStore(ModuleStoreBase):
def __init__(self, host, db, collection, fs_root, render_template,
port=27017, default_class=None,
error_tracker=null_error_tracker,
user=None, password=None, request_cache=None,
metadata_inheritance_cache_subsystem=None, **kwargs):
user=None, password=None, **kwargs):
super(MongoModuleStore, self).__init__()
......@@ -303,8 +302,6 @@ class MongoModuleStore(ModuleStoreBase):
self.error_tracker = error_tracker
self.render_template = render_template
self.ignore_write_events_on_courses = []
self.request_cache = request_cache
self.metadata_inheritance_cache_subsystem = metadata_inheritance_cache_subsystem
def compute_metadata_inheritance_tree(self, location):
'''
......@@ -547,7 +544,7 @@ class MongoModuleStore(ModuleStoreBase):
raise ItemNotFoundError(location)
return item
def has_item(self, location):
def has_item(self, course_id, location):
"""
Returns True if location exists in this ModuleStore.
"""
......@@ -841,6 +838,13 @@ class MongoModuleStore(ModuleStoreBase):
{'_id': True})
return [i['_id'] for i in items]
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
"""
return MONGO_MODULESTORE_TYPE
def _create_new_model_data(self, category, location, definition_data, metadata):
"""
To instantiate a new xmodule which will be saved latter, set up the dbModel and kvs
......@@ -854,9 +858,9 @@ class MongoModuleStore(ModuleStoreBase):
)
class_ = XModuleDescriptor.load_class(
category,
self.default_class
)
category,
self.default_class
)
model_data = DbModel(kvs, class_, None, MongoUsage(None, location))
model_data['category'] = category
model_data['location'] = location
......
......@@ -81,7 +81,7 @@ def path_to_location(modulestore, course_id, location):
# If we're here, there is no path
return None
if not modulestore.has_item(location):
if not modulestore.has_item(course_id, location):
raise ItemNotFoundError
path = find_path_to_course()
......
......@@ -279,7 +279,14 @@ class SplitMongoModuleStore(ModuleStoreBase):
result = self._load_items(course_entry, [root], 0, lazy=True)
return result[0]
def has_item(self, block_location):
def get_course_for_item(self, location):
'''
Provided for backward compatibility. Is equivalent to calling get_course
:param location:
'''
return self.get_course(location)
def has_item(self, course_id, block_location):
"""
Returns True if location exists in its course. Returns false if
the course or the block w/in the course do not exist for the given version.
......
......@@ -146,13 +146,9 @@ def _clone_modules(modulestore, modules, source_location, dest_location):
def clone_course(modulestore, contentstore, source_location, dest_location, delete_original=False):
# first check to see if the modulestore is Mongo backed
if not isinstance(modulestore, MongoModuleStore):
raise Exception("Expected a MongoModuleStore in the runtime. Aborting....")
# check to see if the dest_location exists as an empty course
# we need an empty course because the app layers manage the permissions and users
if not modulestore.has_item(dest_location):
if not modulestore.has_item(dest_location.course_id, dest_location):
raise Exception("An empty course at {0} must have already been created. Aborting...".format(dest_location))
# verify that the dest_location really is an empty course, which means only one with an optional 'overview'
......@@ -171,7 +167,7 @@ def clone_course(modulestore, contentstore, source_location, dest_location, dele
raise Exception("Course at destination {0} is not an empty course. You can only clone into an empty course. Aborting...".format(dest_location))
# check to see if the source course is actually there
if not modulestore.has_item(source_location):
if not modulestore.has_item(source_location.course_id, source_location):
raise Exception("Cannot find a course at {0}. Aborting".format(source_location))
# Get all modules under this namespace which is (tag, org, course) tuple
......@@ -250,7 +246,7 @@ def delete_course(modulestore, contentstore, source_location, commit=False):
"""
# check to see if the source course is actually there
if not modulestore.has_item(source_location):
if not modulestore.has_item(source_location.course_id, source_location):
raise Exception("Cannot find a course at {0}. Aborting".format(source_location))
# first delete all of the thumbnails
......
from nose.tools import assert_equals, assert_raises, assert_false, assert_true, assert_not_equals
import pymongo
from uuid import uuid4
from xmodule.tests import DATA_DIR
from xmodule.modulestore import Location, MONGO_MODULESTORE_TYPE, XML_MODULESTORE_TYPE
from xmodule.modulestore.exceptions import ItemNotFoundError
from xmodule.modulestore.mixed import MixedModuleStore
from xmodule.modulestore.xml_importer import import_from_xml
HOST = 'localhost'
PORT = 27017
DB = 'test_mongo_%s' % uuid4().hex
COLLECTION = 'modulestore'
FS_ROOT = DATA_DIR # TODO (vshnayder): will need a real fs_root for testing load_item
DEFAULT_CLASS = 'xmodule.raw_module.RawDescriptor'
RENDER_TEMPLATE = lambda t_n, d, ctx = None, nsp = 'main': ''
IMPORT_COURSEID = 'MITx/999/2013_Spring'
XML_COURSEID1 = 'edX/toy/2012_Fall'
XML_COURSEID2 = 'edX/simple/2012_Fall'
OPTIONS = {
'mappings': {
XML_COURSEID1: 'xml',
XML_COURSEID2: 'xml',
IMPORT_COURSEID: 'default'
},
'stores': {
'xml': {
'ENGINE': 'xmodule.modulestore.xml.XMLModuleStore',
'OPTIONS': {
'data_dir': DATA_DIR,
'default_class': 'xmodule.hidden_module.HiddenDescriptor',
}
},
'default': {
'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore',
'OPTIONS': {
'default_class': DEFAULT_CLASS,
'host': HOST,
'db': DB,
'collection': COLLECTION,
'fs_root': DATA_DIR,
'render_template': RENDER_TEMPLATE,
}
}
}
}
class TestMixedModuleStore(object):
'''Tests!'''
@classmethod
def setupClass(cls):
cls.connection = pymongo.connection.Connection(HOST, PORT)
cls.connection.drop_database(DB)
cls.fake_location = Location(['i4x', 'foo', 'bar', 'vertical', 'baz'])
cls.import_org, cls.import_course, cls.import_run = IMPORT_COURSEID.split('/')
# NOTE: Creating a single db for all the tests to save time. This
# is ok only as long as none of the tests modify the db.
# If (when!) that changes, need to either reload the db, or load
# once and copy over to a tmp db for each test.
cls.store = cls.initdb()
@classmethod
def teardownClass(cls):
cls.destroy_db(cls.connection)
@staticmethod
def initdb():
# connect to the db
_options = {}
_options.update(OPTIONS)
store = MixedModuleStore(**_options)
import_from_xml(
store._get_modulestore_for_courseid(IMPORT_COURSEID),
DATA_DIR,
['toy'],
target_location_namespace=Location(
'i4x',
TestMixedModuleStore.import_org,
TestMixedModuleStore.import_course,
'course',
TestMixedModuleStore.import_run
)
)
return store
@staticmethod
def destroy_db(connection):
# Destroy the test db.
connection.drop_database(DB)
def setUp(self):
# make a copy for convenience
self.connection = TestMixedModuleStore.connection
def tearDown(self):
pass
def test_get_modulestore_type(self):
"""
Make sure we get back the store type we expect for given mappings
"""
assert_equals(self.store.get_modulestore_type(XML_COURSEID1), XML_MODULESTORE_TYPE)
assert_equals(self.store.get_modulestore_type(XML_COURSEID2), XML_MODULESTORE_TYPE)
assert_equals(self.store.get_modulestore_type(IMPORT_COURSEID), MONGO_MODULESTORE_TYPE)
# try an unknown mapping, it should be the 'default' store
assert_equals(self.store.get_modulestore_type('foo/bar/2012_Fall'), MONGO_MODULESTORE_TYPE)
def test_has_item(self):
assert_true(self.store.has_item(
IMPORT_COURSEID, Location(['i4x', self.import_org, self.import_course, 'course', self.import_run])
))
assert_true(self.store.has_item(
XML_COURSEID1, Location(['i4x', 'edX', 'toy', 'course', '2012_Fall'])
))
# try negative cases
assert_false(self.store.has_item(
XML_COURSEID1, Location(['i4x', self.import_org, self.import_course, 'course', self.import_run])
))
assert_false(self.store.has_item(
IMPORT_COURSEID, Location(['i4x', 'edX', 'toy', 'course', '2012_Fall'])
))
def test_get_item(self):
with assert_raises(NotImplementedError):
self.store.get_item(self.fake_location)
def test_get_instance(self):
module = self.store.get_instance(
IMPORT_COURSEID, Location(['i4x', self.import_org, self.import_course, 'course', self.import_run])
)
assert_not_equals(module, None)
module = self.store.get_instance(
XML_COURSEID1, Location(['i4x', 'edX', 'toy', 'course', '2012_Fall'])
)
assert_not_equals(module, None)
# try negative cases
with assert_raises(ItemNotFoundError):
self.store.get_instance(
XML_COURSEID1, Location(['i4x', self.import_org, self.import_course, 'course', self.import_run])
)
with assert_raises(ItemNotFoundError):
self.store.get_instance(
IMPORT_COURSEID, Location(['i4x', 'edX', 'toy', 'course', '2012_Fall'])
)
def test_get_items(self):
modules = self.store.get_items(['i4x', None, None, 'course', None], IMPORT_COURSEID)
assert_equals(len(modules), 1)
assert_equals(modules[0].location.course, self.import_course)
modules = self.store.get_items(['i4x', None, None, 'course', None], XML_COURSEID1)
assert_equals(len(modules), 1)
assert_equals(modules[0].location.course, 'toy')
modules = self.store.get_items(['i4x', None, None, 'course', None], XML_COURSEID2)
assert_equals(len(modules), 1)
assert_equals(modules[0].location.course, 'simple')
def test_update_item(self):
with assert_raises(NotImplementedError):
self.store.update_item(self.fake_location, None)
def test_update_children(self):
with assert_raises(NotImplementedError):
self.store.update_children(self.fake_location, None)
def test_update_metadata(self):
with assert_raises(NotImplementedError):
self.store.update_metadata(self.fake_location, None)
def test_delete_item(self):
with assert_raises(NotImplementedError):
self.store.delete_item(self.fake_location)
def test_get_courses(self):
# we should have 3 total courses aggregated
courses = self.store.get_courses()
assert_equals(len(courses), 3)
course_ids = []
for course in courses:
course_ids.append(course.location.course_id)
assert_true(IMPORT_COURSEID in course_ids)
assert_true(XML_COURSEID1 in course_ids)
assert_true(XML_COURSEID2 in course_ids)
def test_get_course(self):
module = self.store.get_course(IMPORT_COURSEID)
assert_equals(module.location.course, self.import_course)
module = self.store.get_course(XML_COURSEID1)
assert_equals(module.location.course, 'toy')
module = self.store.get_course(XML_COURSEID2)
assert_equals(module.location.course, 'simple')
def test_get_parent_locations(self):
parents = self.store.get_parent_locations(
Location(['i4x', self.import_org, self.import_course, 'chapter', 'Overview']),
IMPORT_COURSEID
)
assert_equals(len(parents), 1)
assert_equals(Location(parents[0]).org, self.import_org)
assert_equals(Location(parents[0]).course, self.import_course)
assert_equals(Location(parents[0]).name, self.import_run)
parents = self.store.get_parent_locations(
Location(['i4x', 'edX', 'toy', 'chapter', 'Overview']),
XML_COURSEID1
)
assert_equals(len(parents), 1)
assert_equals(Location(parents[0]).org, 'edX')
assert_equals(Location(parents[0]).course, 'toy')
assert_equals(Location(parents[0]).name, '2012_Fall')
def test_set_modulestore_configuration(self):
config = {'foo': 'bar'}
self.store.set_modulestore_configuration(config)
assert_equals(
config,
self.store._get_modulestore_for_courseid(IMPORT_COURSEID).modulestore_configuration
)
assert_equals(
config,
self.store._get_modulestore_for_courseid(XML_COURSEID1).modulestore_configuration
)
assert_equals(
config,
self.store._get_modulestore_for_courseid(XML_COURSEID2).modulestore_configuration
)
......@@ -45,8 +45,7 @@ class TestMongoModuleStore(object):
@staticmethod
def initdb():
# connect to the db
store = MongoModuleStore(HOST, DB, COLLECTION, FS_ROOT, RENDER_TEMPLATE,
default_class=DEFAULT_CLASS)
store = MongoModuleStore(HOST, DB, COLLECTION, FS_ROOT, RENDER_TEMPLATE, default_class=DEFAULT_CLASS)
# Explicitly list the courses to load (don't want the big one)
courses = ['toy', 'simple']
import_from_xml(store, DATA_DIR, courses)
......@@ -71,6 +70,10 @@ class TestMongoModuleStore(object):
pprint([Location(i['_id']).url() for i in ids])
def test_mongo_modulestore_type(self):
store = MongoModuleStore(HOST, DB, COLLECTION, FS_ROOT, RENDER_TEMPLATE, default_class=DEFAULT_CLASS)
assert_equals(store.get_modulestore_type('foo/bar/baz'), 'mongo')
def test_get_courses(self):
'''Make sure the course objects loaded properly'''
courses = self.store.get_courses()
......@@ -117,6 +120,7 @@ class TestMongoModuleStore(object):
'{0} is a template course'.format(course)
)
class TestMongoKeyValueStore(object):
def setUp(self):
......
......@@ -258,18 +258,19 @@ class SplitModuleItemTests(SplitModuleTest):
'''
has_item(BlockUsageLocator)
'''
course_id = 'GreekHero'
# positive tests of various forms
locator = BlockUsageLocator(version_guid=self.GUID_D1, usage_id='head12345')
self.assertTrue(modulestore().has_item(locator),
self.assertTrue(modulestore().has_item(course_id, locator),
"couldn't find in %s" % self.GUID_D1)
locator = BlockUsageLocator(course_id='GreekHero', usage_id='head12345', branch='draft')
self.assertTrue(
modulestore().has_item(locator),
modulestore().has_item(locator.course_id, locator),
"couldn't find in 12345"
)
self.assertTrue(
modulestore().has_item(BlockUsageLocator(
modulestore().has_item(locator.course_id, BlockUsageLocator(
course_id=locator.course_id,
branch='draft',
usage_id=locator.usage_id
......@@ -277,7 +278,7 @@ class SplitModuleItemTests(SplitModuleTest):
"couldn't find in draft 12345"
)
self.assertFalse(
modulestore().has_item(BlockUsageLocator(
modulestore().has_item(locator.course_id, BlockUsageLocator(
course_id=locator.course_id,
branch='published',
usage_id=locator.usage_id)),
......@@ -285,40 +286,43 @@ class SplitModuleItemTests(SplitModuleTest):
)
locator.branch = 'draft'
self.assertTrue(
modulestore().has_item(locator),
modulestore().has_item(locator.course_id, locator),
"not found in draft 12345"
)
# not a course obj
locator = BlockUsageLocator(course_id='GreekHero', usage_id='chapter1', branch='draft')
self.assertTrue(
modulestore().has_item(locator),
modulestore().has_item(locator.course_id, locator),
"couldn't find chapter1"
)
# in published course
locator = BlockUsageLocator(course_id="wonderful", usage_id="head23456", branch='draft')
self.assertTrue(modulestore().has_item(BlockUsageLocator(course_id=locator.course_id,
usage_id=locator.usage_id,
branch='published')),
"couldn't find in 23456")
self.assertTrue(
modulestore().has_item(
locator.course_id,
BlockUsageLocator(course_id=locator.course_id, usage_id=locator.usage_id, branch='published')
), "couldn't find in 23456"
)
locator.branch = 'published'
self.assertTrue(modulestore().has_item(locator), "couldn't find in 23456")
self.assertTrue(modulestore().has_item(course_id, locator), "couldn't find in 23456")
def test_negative_has_item(self):
# negative tests--not found
# no such course or block
course_id = 'GreekHero'
locator = BlockUsageLocator(course_id="doesnotexist", usage_id="head23456", branch='draft')
self.assertFalse(modulestore().has_item(locator))
self.assertFalse(modulestore().has_item(course_id, locator))
locator = BlockUsageLocator(course_id="wonderful", usage_id="doesnotexist", branch='draft')
self.assertFalse(modulestore().has_item(locator))
self.assertFalse(modulestore().has_item(course_id, locator))
# negative tests--insufficient specification
self.assertRaises(InsufficientSpecificationError, BlockUsageLocator)
self.assertRaises(InsufficientSpecificationError,
modulestore().has_item, BlockUsageLocator(version_guid=self.GUID_D1))
modulestore().has_item, None, BlockUsageLocator(version_guid=self.GUID_D1))
self.assertRaises(InsufficientSpecificationError,
modulestore().has_item, BlockUsageLocator(course_id='GreekHero'))
modulestore().has_item, None, BlockUsageLocator(course_id='GreekHero'))
def test_get_item(self):
'''
......@@ -738,13 +742,13 @@ class TestItemCrud(SplitModuleTest):
deleted = BlockUsageLocator(course_id=reusable_location.course_id,
branch=reusable_location.branch,
usage_id=locn_to_del.usage_id)
self.assertFalse(modulestore().has_item(deleted))
self.assertRaises(VersionConflictError, modulestore().has_item, locn_to_del)
self.assertFalse(modulestore().has_item(reusable_location.course_id, deleted))
self.assertRaises(VersionConflictError, modulestore().has_item, reusable_location.course_id, locn_to_del)
locator = BlockUsageLocator(
version_guid=locn_to_del.version_guid,
usage_id=locn_to_del.usage_id
)
self.assertTrue(modulestore().has_item(locator))
self.assertTrue(modulestore().has_item(reusable_location.course_id, locator))
self.assertNotEqual(new_course_loc.version_guid, course.location.version_guid)
# delete a subtree
......@@ -755,7 +759,7 @@ class TestItemCrud(SplitModuleTest):
def check_subtree(node):
if node:
node_loc = node.location
self.assertFalse(modulestore().has_item(
self.assertFalse(modulestore().has_item(reusable_location.course_id,
BlockUsageLocator(
course_id=node_loc.course_id,
branch=node_loc.branch,
......@@ -763,7 +767,7 @@ class TestItemCrud(SplitModuleTest):
locator = BlockUsageLocator(
version_guid=node.location.version_guid,
usage_id=node.location.usage_id)
self.assertTrue(modulestore().has_item(locator))
self.assertTrue(modulestore().has_item(reusable_location.course_id, locator))
if node.has_children:
for sub in node.get_children():
check_subtree(sub)
......@@ -874,7 +878,7 @@ class TestCourseCreation(SplitModuleTest):
original_course = modulestore().get_course(original_locator)
self.assertEqual(original_course.location.version_guid, original_index['versions']['draft'])
self.assertFalse(
modulestore().has_item(BlockUsageLocator(
modulestore().has_item(new_draft_locator.course_id, BlockUsageLocator(
original_locator,
usage_id=new_item.location.usage_id
))
......
import os.path
from nose.tools import assert_raises
from nose.tools import assert_raises, assert_equals
from xmodule.course_module import CourseDescriptor
from xmodule.modulestore.xml import XMLModuleStore
from xmodule.modulestore import XML_MODULESTORE_TYPE
from .test_modulestore import check_path_to_location
from xmodule.tests import DATA_DIR
from xmodule.modulestore.tests.test_modulestore import check_path_to_location
class TestXMLModuleStore(object):
......@@ -19,6 +20,10 @@ class TestXMLModuleStore(object):
check_path_to_location(modulestore)
def test_xml_modulestore_type(self):
store = XMLModuleStore(DATA_DIR, course_dirs=['toy', 'simple'])
assert_equals(store.get_modulestore_type('foo/bar/baz'), XML_MODULESTORE_TYPE)
def test_unicode_chars_in_xml_content(self):
# edX/full/6.002_Spring_2012 has non-ASCII chars, and during
# uniquification of names, would raise a UnicodeError. It no longer does.
......
......@@ -21,7 +21,7 @@ from xmodule.x_module import XModuleDescriptor, XMLParsingSystem
from xmodule.html_module import HtmlDescriptor
from . import ModuleStoreBase, Location
from . import ModuleStoreBase, Location, XML_MODULESTORE_TYPE
from .exceptions import ItemNotFoundError
from .inheritance import compute_inherited_metadata
......@@ -505,12 +505,12 @@ class XMLModuleStore(ModuleStoreBase):
except KeyError:
raise ItemNotFoundError(location)
def has_item(self, location):
def has_item(self, course_id, location):
"""
Returns True if location exists in this ModuleStore.
"""
location = Location(location)
return any(location in course_modules for course_modules in self.modules.values())
return location in self.modules[course_id]
def get_item(self, location, depth=0):
"""
......@@ -601,3 +601,10 @@ class XMLModuleStore(ModuleStoreBase):
raise ItemNotFoundError("{0} not in {1}".format(location, course_id))
return self.parent_trackers[course_id].parents(location)
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
"""
return XML_MODULESTORE_TYPE
......@@ -19,12 +19,18 @@ class ContentTest(unittest.TestCase):
content = StaticContent('loc', 'name', 'content_type', 'data')
self.assertIsNone(content.thumbnail_location)
def test_static_url_generation_from_courseid(self):
url = StaticContent.convert_legacy_static_url_with_course_id('images_course_image.jpg', 'foo/bar/bz')
self.assertEqual(url, '/c4x/foo/bar/asset/images_course_image.jpg')
def test_generate_thumbnail_image(self):
contentStore = ContentStore()
content = Content(Location(u'c4x', u'mitX', u'800', u'asset', u'monsters__.jpg'), None)
(thumbnail_content, thumbnail_file_location) = contentStore.generate_thumbnail(content)
self.assertIsNone(thumbnail_content)
self.assertEqual(Location(u'c4x', u'mitX', u'800', u'thumbnail', u'monsters__.jpg'), thumbnail_file_location)
def test_compute_location(self):
# We had a bug that __ got converted into a single _. Make sure that substitution of INVALID_CHARS (like space)
# still happen.
......
......@@ -161,12 +161,7 @@ class VideoModule(VideoFields, XModule):
return json.dumps({'position': self.position})
def get_html(self):
if isinstance(modulestore(), MongoModuleStore):
caption_asset_path = StaticContent.get_base_url_path_for_course_assets(self.location) + '/subs_'
else:
# VS[compat]
# cdodge: filesystem static content support.
caption_asset_path = "/static/subs/"
caption_asset_path = "/static/subs/"
get_ext = lambda filename: filename.rpartition('.')[-1]
sources = {get_ext(src): src for src in self.html5_sources}
......
from xmodule.modulestore.django import modulestore
from xmodule.course_module import CourseDescriptor
from django.conf import settings
......@@ -15,7 +14,9 @@ def get_visible_courses(domain=None):
"""
Return the set of CourseDescriptors that should be visible in this branded instance
"""
courses = [c for c in modulestore().get_courses()
_courses = modulestore().get_courses()
courses = [c for c in _courses
if isinstance(c, CourseDescriptor)]
courses = sorted(courses, key=lambda course: course.number)
......
......@@ -8,10 +8,9 @@ from django.http import Http404
from .module_render import get_module
from xmodule.course_module import CourseDescriptor
from xmodule.modulestore import Location
from xmodule.modulestore import Location, XML_MODULESTORE_TYPE
from xmodule.modulestore.django import modulestore
from xmodule.contentstore.content import StaticContent
from xmodule.modulestore.xml import XMLModuleStore
from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError
from courseware.model_data import ModelDataCache
from static_replace import replace_static_urls
......@@ -82,12 +81,12 @@ def get_opt_course_with_access(user, course_id, action):
def course_image_url(course):
"""Try to look up the image url for the course. If it's not found,
log an error and return the dead link"""
if isinstance(modulestore(), XMLModuleStore):
if modulestore().get_modulestore_type(course.location.course_id) == XML_MODULESTORE_TYPE:
return '/static/' + course.data_dir + "/images/course_image.jpg"
else:
loc = course.location._replace(tag='c4x', category='asset', name='images_course_image.jpg')
path = StaticContent.get_url_path_from_location(loc)
return path
_path = StaticContent.get_url_path_from_location(loc)
return _path
def find_file(fs, dirs, filename):
......@@ -243,7 +242,7 @@ def get_course_syllabus_section(course, section_key):
return replace_static_urls(
htmlFile.read().decode('utf-8'),
getattr(course, 'data_dir', None),
course_namespace=course.location
course_id=course.location.course_id
)
except ResourceNotFoundError:
log.exception("Missing syllabus section {key} in course {url}".format(
......
......@@ -332,6 +332,7 @@ def get_module_for_descriptor_internal(user, descriptor, model_data_cache, cours
# TODO (cpennington): When modules are shared between courses, the static
# prefix is going to have to be specific to the module, not the directory
# that the xml was loaded from
system = ModuleSystem(
track_function=track_function,
render_template=render_to_string,
......@@ -347,7 +348,7 @@ def get_module_for_descriptor_internal(user, descriptor, model_data_cache, cours
replace_urls=partial(
static_replace.replace_static_urls,
data_directory=getattr(descriptor, 'data_dir', None),
course_namespace=descriptor.location._replace(category=None, name=None),
course_id=course_id,
),
replace_course_urls=partial(
static_replace.replace_course_urls,
......@@ -368,6 +369,7 @@ def get_module_for_descriptor_internal(user, descriptor, model_data_cache, cours
cache=cache,
can_execute_unsafe_code=(lambda: can_execute_unsafe_code(course_id)),
)
# pass position specified in URL to module through ModuleSystem
system.set('position', position)
system.set('DEBUG', settings.DEBUG)
......@@ -405,7 +407,7 @@ def get_module_for_descriptor_internal(user, descriptor, model_data_cache, cours
module.get_html = replace_static_urls(
_get_html,
getattr(descriptor, 'data_dir', None),
course_namespace=module.location._replace(category=None, name=None)
course_id=course_id
)
# Allow URLs of the form '/course/' refer to the root of multicourse directory
......
......@@ -50,8 +50,7 @@ class BaseTestXmodule(ModuleStoreTestCase):
self.course = CourseFactory.create(data=self.COURSE_DATA)
# Turn off cache.
modulestore().request_cache = None
modulestore().metadata_inheritance_cache_subsystem = None
modulestore().set_modulestore_configuration({})
chapter = ItemFactory.create(
parent_location=self.course.location,
......
......@@ -54,7 +54,7 @@ class TestVideo(BaseTestXmodule):
expected_context = {
'data_dir': getattr(self, 'data_dir', None),
'caption_asset_path': '/c4x/MITx/999/asset/subs_',
'caption_asset_path': '/static/subs/',
'show_captions': 'true',
'display_name': 'A Name',
'end': 3610.0,
......@@ -104,7 +104,7 @@ class TestVideoNonYouTube(TestVideo):
expected_context = {
'data_dir': getattr(self, 'data_dir', None),
'caption_asset_path': '/c4x/MITx/999/asset/subs_',
'caption_asset_path': '/static/subs/',
'show_captions': 'true',
'display_name': 'A Name',
'end': 3610.0,
......
......@@ -25,7 +25,8 @@ class TestGradebook(ModuleStoreTestCase):
instructor = AdminFactory.create()
self.client.login(username=instructor.username, password='test')
modulestore().request_cache = modulestore().metadata_inheritance_cache_subsystem = None
# remove the caches
modulestore().set_modulestore_configuration({})
kwargs = {}
if self.grading_policy is not None:
......
......@@ -50,7 +50,7 @@ def remap_static_url(original_url, course):
output_url = replace_static_urls(
input_url,
getattr(course, 'data_dir', None),
course_namespace=course.location,
course_id=course.location.course_id,
)
# strip off the quotes again...
return output_url[1:-1]
......
"""
This configuration is to run the MixedModuleStore on a localdev environment
"""
# We intentionally define lots of variables that aren't used, and
# want to import all variables from base settings files
# pylint: disable=W0401, W0614
from .dev import *, DATA_DIR
MODULESTORE = {
'default': {
'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore',
'OPTIONS': {
'mappings': {
'MITx/2.01x/2013_Spring': 'xml'
},
'stores': {
'xml': {
'ENGINE': 'xmodule.modulestore.xml.XMLModuleStore',
'OPTIONS': {
'data_dir': DATA_DIR,
'default_class': 'xmodule.hidden_module.HiddenDescriptor',
}
},
'default': {
'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore',
'OPTIONS': {
'default_class': 'xmodule.raw_module.RawDescriptor',
'host': 'localhost',
'db': 'xmodule',
'collection': 'modulestore',
'fs_root': DATA_DIR,
'render_template': 'mitxmako.shortcuts.render_to_string',
}
}
},
}
}
}
......@@ -8,8 +8,10 @@ from django.core.cache import get_cache
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()
})
if hasattr(settings, 'DATADOG_API'):
dog_http_api.api_key = settings.DATADOG_API
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment