Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
1b111b1
add ability to import course (into CMS / edge) without static content,
ichuang Aug 12, 2013
a5bb971
add static_asset_path metadata to course, and honor its use in link r…
ichuang Aug 12, 2013
220ec52
Merge branch 'master' of github.com:edx/edx-platform into feature/ich…
ichuang Aug 19, 2013
39b6465
pep8 in inheritance.py
ichuang Aug 19, 2013
13bb3bf
pylint
ichuang Aug 19, 2013
2249692
pylint
ichuang Aug 19, 2013
e9ef450
pylint
ichuang Aug 19, 2013
210fa11
modify handling of info/handouts and module_render to honor static_as…
ichuang Aug 20, 2013
2fe4895
more static_asset_path handling in courses.py
ichuang Aug 20, 2013
fedfa7c
fix tabs.py to properly handle static_asset_path
ichuang Aug 20, 2013
91bf6ad
remove extra debugging line from courses.py
ichuang Aug 20, 2013
95952bd
add tests of static_asset_path and importing with no static;
ichuang Aug 21, 2013
0d938d3
Merge branch 'master' of github.com:edx/edx-platform into feature/ich…
ichuang Aug 21, 2013
8a84e67
clean up test_import_nostatic.py
ichuang Aug 21, 2013
d2e93f7
remove unncessary images from new test course
ichuang Aug 21, 2013
9b26e5b
dummy commit - trigger jenkins rebuild
ichuang Aug 21, 2013
42af561
pep8 and pylint for tests of nostatic import
ichuang Aug 21, 2013
2ba0d40
fix pep8 violations
Aug 22, 2013
ef98c54
fix some pylint violations
Aug 22, 2013
fb3a5bf
remove unused class
Aug 22, 2013
0c1c3f1
loop in static import testing into common/* tests
Aug 22, 2013
77a38af
add some draft courseware importing paths in common/* tests
Aug 22, 2013
842556d
add new test data with draft content
Aug 22, 2013
079470d
add some more tests in common to increase coverage
Aug 22, 2013
7138aed
add xlint tests in test_mongo.py
Aug 22, 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
16 changes: 12 additions & 4 deletions cms/djangoapps/contentstore/management/commands/import.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
Script for importing courseware from XML format
"""

from django.core.management.base import BaseCommand, CommandError
from django.core.management.base import BaseCommand, CommandError, make_option
from xmodule.modulestore.xml_importer import import_from_xml
from xmodule.modulestore.django import modulestore
from xmodule.contentstore.django import contentstore
Expand All @@ -14,18 +14,26 @@ class Command(BaseCommand):
"""
help = 'Import the specified data directory into the default ModuleStore'

option_list = BaseCommand.option_list + (
make_option('--nostatic',
action='store_true',
help='Skip import of static content'),
)

def handle(self, *args, **options):
"Execute the command"
if len(args) == 0:
raise CommandError("import requires at least one argument: <data directory> [<course dir>...]")
raise CommandError("import requires at least one argument: <data directory> [--nostatic] [<course dir>...]")

data_dir = args[0]
do_import_static = not (options.get('nostatic', False))
if len(args) > 1:
course_dirs = args[1:]
else:
course_dirs = None
print("Importing. Data_dir={data}, course_dirs={courses}".format(
data=data_dir,
courses=course_dirs))
courses=course_dirs,
dis=do_import_static))
import_from_xml(modulestore('direct'), data_dir, course_dirs, load_error_modules=False,
static_content_store=contentstore(), verbose=True)
static_content_store=contentstore(), verbose=True, do_import_static=do_import_static)

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 your plan to not use Mongo (via GridFS) for your static resources but serve those off filesystem?

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.

Yes - that is the main point of this PR. There are courses with huge
static resource collections, which take too long to import. And the gridfs
contentstore doest allow hierarchial storage.

This PR is already operating in production at MIT.
On Aug 19, 2013 9:22 AM, "chrisndodge" notifications@github.com wrote:

In cms/djangoapps/contentstore/management/commands/import.py:

     import_from_xml(modulestore('direct'), data_dir, course_dirs, load_error_modules=False,
  •                    static_content_store=contentstore(), verbose=True)
    
  •                    static_content_store=contentstore(), verbose=True, do_import_static=do_import_static)
    

Is your plan to not use Mongo (via GridFS) for your static resources
but serve those off filesystem?

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/652/files#r5841597
.

7 changes: 4 additions & 3 deletions common/djangoapps/static_replace/__init__.py
Original file line number Diff line number Diff line change
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_id=None):
def replace_static_urls(text, data_directory, course_id=None, static_asset_path=''):
"""
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 @@ -100,6 +100,7 @@ def replace_static_urls(text, data_directory, course_id=None):
text: The source text to do the substitution in
data_directory: The directory in which course data is stored
course_id: The course identifier used to distinguish static content for this course in studio
static_asset_path: Path for static assets, which overrides data_directory and course_namespace, if nonempty
"""

def replace_static_url(match):
Expand Down Expand Up @@ -127,7 +128,7 @@ def replace_static_url(match):
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))
course_path = "/".join((static_asset_path or data_directory, rest))

try:
if staticfiles_storage.exists(rest):
Expand All @@ -143,7 +144,7 @@ def replace_static_url(match):
return "".join([quote, url, quote])

return re.sub(
_url_replace_regex('/static/(?!{data_dir})'.format(data_dir=data_directory)),
_url_replace_regex('/static/(?!{data_dir})'.format(data_dir=static_asset_path or data_directory)),
replace_static_url,
text
)
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_id=None):
def replace_static_urls(get_html, data_dir, course_id=None, static_asset_path=''):
"""
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_id=None):

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


Expand Down
3 changes: 2 additions & 1 deletion common/lib/xmodule/xmodule/modulestore/inheritance.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
# intended to be set per-course, but can be overridden in for specific
# elements. Can be a float.
'days_early_for_beta',
'giturl' # for git edit link
'giturl', # for git edit link
'static_asset_path', # for static assets placed outside xcontent contentstore
)


Expand Down
51 changes: 44 additions & 7 deletions common/lib/xmodule/xmodule/modulestore/xml_importer.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,10 @@ def import_static_content(modules, course_loc, course_data_path, static_content_
content.thumbnail_location = thumbnail_location

#then commit the content
static_content_store.save(content)
try:
static_content_store.save(content)
except Exception as err:
log.exception('Error importing {0}, error={1}'.format(fullname_with_subpath, err))

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.

Sorry, just noticing this. Why the generic try/catch? What errors have you seen here?

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.

Actually, you already commented on it once and I replied. We were observing a bunch of errors importing courses a few months ago, when we added this; I don't recall what the exceptions were, but there were several kinds.

The idea is that such errors really ought to be surfaced to users, and not terminate the import. Also, without logging the exception, the filename of the file causing the error is unknown. This at least surfaces the error to our production team (since they have access to the logs).

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.

Ha! That could be. We're all doing a lot of things and it's easy to loose track.

I'm adding some test to bump up the coverage numbers on xml_import.py.

I have to talk to @jzoldak about a few items, because a lot of xml_import is getting exercised in the CMS tests, but I don't think that is getting "counted" as coverage on the common/* reports. So this is a gap in my understanding.

So in the meantime, I'm trying to exercise more code paths in the common/* tests. Should get more done tonight or early tomorrow.


#store the remapping information which will be needed to subsitute in the module data
remap_dict[fullname_with_subpath] = content_loc.name
Expand All @@ -64,7 +67,8 @@ def import_static_content(modules, course_loc, course_data_path, static_content_
def import_from_xml(store, data_dir, course_dirs=None,
default_class='xmodule.raw_module.RawDescriptor',
load_error_modules=True, static_content_store=None, target_location_namespace=None,
verbose=False, draft_store=None):
verbose=False, draft_store=None,
do_import_static=True):
"""
Import the specified xml data_dir into the "store" modulestore,
using org and course as the location org and course.
Expand All @@ -77,6 +81,10 @@ def import_from_xml(store, data_dir, course_dirs=None,
expects a 'url_name' as an identifier to where things are on disk e.g. ../policies/<url_name>/policy.json as well as metadata keys in
the policy.json. so we need to keep the original url_name during import

do_import_static: if False, then static files are not imported into the static content store. This can be employed for courses which
have substantial unchanging static content, which is to inefficient to import every time the course is loaded.
Static content for some courses may also be served directly by nginx, instead of going through django.

"""

xml_module_store = XMLModuleStore(
Expand Down Expand Up @@ -116,8 +124,17 @@ def import_from_xml(store, data_dir, course_dirs=None,
course_data_path = path(data_dir) / module.data_dir
course_location = module.location

log.debug('======> IMPORTING course to location {0}'.format(course_location))

module = remap_namespace(module, target_location_namespace)

if not do_import_static:
module.lms.static_asset_path = module.data_dir # for old-style xblock where this was actually linked to kvs
module._model_data['static_asset_path'] = module.data_dir
log.debug('course static_asset_path={0}'.format(module.lms.static_asset_path))

log.debug('course data_dir={0}'.format(module.data_dir))

# cdodge: more hacks (what else). Seems like we have a problem when importing a course (like 6.002) which
# does not have any tabs defined in the policy file. The import goes fine and then displays fine in LMS,
# but if someone tries to add a new tab in the CMS, then the LMS barfs because it expects that -
Expand All @@ -129,18 +146,36 @@ def import_from_xml(store, data_dir, course_dirs=None,
{"type": "wiki", "name": "Wiki"}] # note, add 'progress' when we can support it on Edge

import_module(module, store, course_data_path, static_content_store, course_location,
target_location_namespace or course_location)
target_location_namespace or course_location, do_import_static=do_import_static)

course_items.append(module)

# then import all the static content
if static_content_store is not None:
if static_content_store is not None and do_import_static:
_namespace_rename = target_location_namespace if target_location_namespace is not None else course_location

# first pass to find everything in /static/
import_static_content(xml_module_store.modules[course_id], course_location, course_data_path, static_content_store,
_namespace_rename, subpath='static', verbose=verbose)

elif verbose and not do_import_static:
log.debug('Skipping import of static content, since do_import_static={0}'.format(do_import_static))

# no matter what do_import_static is, import "static_import" directory

# This is needed because the "about" pages (eg "overview") are loaded via load_extra_content, and
# do not inherit the lms metadata from the course module, and thus do not get "static_content_store"
# properly defined. Static content referenced in those extra pages thus need to come through the
# c4x:// contentstore, unfortunately. Tell users to copy that content into the "static_import" subdir.

simport = 'static_import'
if os.path.exists(course_data_path / simport):

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.

Just so I understand the usage here, XML writers will be advised to put static content into a folder named 'static_import' which is known to have to be referenced by modules (in the link rewriting). Correct?

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.

Correct. This only affects a few special pages. Ideally, the way those special pages are handled through load_extra_content would eventually be improved so that they would honor also static_asset_path, and make it unnecessary to use static_import.

_namespace_rename = target_location_namespace if target_location_namespace is not None else course_location

import_static_content(xml_module_store.modules[course_id], course_location, course_data_path, static_content_store,
_namespace_rename, subpath=simport, verbose=verbose)


# finally loop through all the modules
for module in xml_module_store.modules[course_id].itervalues():
if module.category == 'course':
Expand All @@ -156,7 +191,8 @@ def import_from_xml(store, data_dir, course_dirs=None,
log.debug('importing module location {0}'.format(module.location))

import_module(module, store, course_data_path, static_content_store, course_location,
target_location_namespace if target_location_namespace else course_location)
target_location_namespace if target_location_namespace else course_location,
do_import_static=do_import_static)

# now import any 'draft' items
if draft_store is not None:
Expand All @@ -176,7 +212,8 @@ def import_from_xml(store, data_dir, course_dirs=None,


def import_module(module, store, course_data_path, static_content_store,
source_course_location, dest_course_location, allow_not_found=False):
source_course_location, dest_course_location, allow_not_found=False,
do_import_static=True):

logging.debug('processing import of module {0}...'.format(module.location.url()))

Expand All @@ -196,7 +233,7 @@ def import_module(module, store, course_data_path, static_content_store,
else:
module_data = content

if isinstance(module_data, basestring):
if isinstance(module_data, basestring) and do_import_static:
# we want to convert all 'non-portable' links in the module_data (if it is a string) to
# portable strings (e.g. /static/)
module_data = rewrite_nonportable_content_links(
Expand Down
2 changes: 1 addition & 1 deletion lms/djangoapps/courseware/courses.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ 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 modulestore().get_modulestore_type(course.location.course_id) == XML_MODULESTORE_TYPE:
return '/static/' + course.data_dir + "/images/course_image.jpg"
return '/static/' + (course.lms.static_asset_path or getattr(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)
Expand Down
4 changes: 3 additions & 1 deletion lms/djangoapps/courseware/module_render.py
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,7 @@ def publish(event):
static_replace.replace_static_urls,
data_directory=getattr(descriptor, 'data_dir', None),
course_id=course_id,
static_asset_path=descriptor.lms.static_asset_path,
),
replace_course_urls=partial(
static_replace.replace_course_urls,
Expand Down Expand Up @@ -407,7 +408,8 @@ def publish(event):
module.get_html = replace_static_urls(
_get_html,
getattr(descriptor, 'data_dir', None),
course_id=course_id
course_id=course_id,
static_asset_path=descriptor.lms.static_asset_path
)

# Allow URLs of the form '/course/' refer to the root of multicourse directory
Expand Down
1 change: 1 addition & 0 deletions lms/xmodule_namespace.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,4 @@ class LmsNamespace(Namespace):
default=None,
scope=Scope.settings
)
static_asset_path = String(help="Path to use for static assets - overrides Studio c4x://", scope=Scope.settings, default='')