From ac010795021e8e9103992b4b77c0ad51ba52aac9 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 4 Apr 2013 09:35:35 -0400 Subject: [PATCH 1/5] add a command line utility to walk through the course and find common errors --- .../management/commands/check_course.py | 74 +++++++++++++++++++ .../xmodule/modulestore/xml_importer.py | 42 ++++++++--- 2 files changed, 105 insertions(+), 11 deletions(-) create mode 100644 cms/djangoapps/contentstore/management/commands/check_course.py diff --git a/cms/djangoapps/contentstore/management/commands/check_course.py b/cms/djangoapps/contentstore/management/commands/check_course.py new file mode 100644 index 0000000000..c691ac63a3 --- /dev/null +++ b/cms/djangoapps/contentstore/management/commands/check_course.py @@ -0,0 +1,74 @@ +from django.core.management.base import BaseCommand, CommandError +from xmodule.modulestore.django import modulestore +from xmodule.contentstore.django import contentstore +from xmodule.modulestore.xml_importer import check_module_metadata_editability +from xmodule.course_module import CourseDescriptor + +from request_cache.middleware import RequestCache + +from django.core.cache import get_cache, InvalidCacheBackendError + +class Command(BaseCommand): + help = '''Delete a MongoDB backed course''' + + def handle(self, *args, **options): + if len(args) != 1 : + raise CommandError("check_course requires one argument: ") + + loc_str = args[0] + + loc = CourseDescriptor.id_to_location(loc_str) + + ms = modulestore() + + # setup a request cache so we don't throttle the DB with all the metadata inheritance requests + ms.request_cache = RequestCache.get_request_cache() + + course = ms.get_item(loc, depth=3) + + err_cnt = 0 + def _xlint_metadata(module): + err_cnt = check_module_metadata_editability(module) + for child in module.get_children(): + err_cnt = err_cnt + _xlint_metadata(child) + return err_cnt + + _xlint_metadata(course) + + # we've had a bug where the xml_attributes field can we rewritten as a string rather than a dict + def _check_xml_attributes_field(module): + err_cnt = 0 + if hasattr(module, 'xml_attributes') and (isinstance(module, str) or isinstance(module, unicode)): + print 'module = {0} has xml_attributes as a string. It should be a dict'.format(module.location.url()) + err_cnt = err_cnt + 1 + for child in module.get_children(): + err_cnt = err_cnt + _check_xml_attributes_field(child) + return err_cnt + + _check_xml_attributes_field(course) + + # check for dangling discussion items, this can cause errors in the forums + def _get_discussion_items(module): + discussion_items = [] + if module.location.category == 'discussion': + discussion_items = discussion_items + [module.location.url()] + + for child in module.get_children(): + discussion_items = discussion_items + _get_discussion_items(child) + + return discussion_items + + discussion_items =_get_discussion_items(course) + + # now query all discussion items via get_items() and compare with the tree-traversal + queried_discussion_items = ms.get_items(['i4x', course.location.org, course.location.course, + 'discussion', None, None]) + + for item in queried_discussion_items: + if item.location.url() not in discussion_items: + print 'Found dangling discussion module = {0}'.format(item.location.url()) + + + + + diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 7c1f1fb4f7..0a7d07afe1 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -356,22 +356,43 @@ def remap_namespace(module, target_location_namespace): return module -def validate_no_non_editable_metadata(module_store, course_id, category, allowed=None): + +def allowed_metadata_by_category(category): + # should this be in the descriptors?!? + return { + 'vertical': [], + 'chapter': ['start'], + 'sequential': ['due', 'format', 'start', 'graded'] + }.get(category,['*']) + + +def check_module_metadata_editability(module): ''' - Assert that there is no metadata within a particular category that we can't support editing + Assert that there is no metadata within a particular module that we can't support editing However we always allow 'display_name' and 'xml_attribtues' ''' - _allowed = (allowed if allowed is not None else []) + ['xml_attributes', 'display_name'] + allowed = allowed_metadata_by_category(module.location.category) + if '*' in allowed: + # everything is allowed + return 0 + allowed = allowed + ['xml_attributes', 'display_name'] + err_cnt = 0 + my_metadata = dict(own_metadata(module)) + for key in my_metadata.keys(): + if key not in allowed: + err_cnt = err_cnt + 1 + print ': found metadata on {0}. Studio will not support editing this piece of metadata, so it is not allowed. Metadata: {1} = {2}'. format(module.location.url(), key, my_metadata[key]) + + return err_cnt + + +def validate_no_non_editable_metadata(module_store, course_id, category): err_cnt = 0 for module_loc in module_store.modules[course_id]: module = module_store.modules[course_id][module_loc] if module.location.category == category: - my_metadata = dict(own_metadata(module)) - for key in my_metadata.keys(): - if key not in _allowed: - err_cnt = err_cnt + 1 - print ': found metadata on {0}. Studio will not support editing this piece of metadata, so it is not allowed. Metadata: {1} = {2}'. format(module.location.url(), key, my_metadata[key]) + err_cnt = err_cnt + check_module_metadata_editability(module) return err_cnt @@ -463,10 +484,9 @@ def perform_xlint(data_dir, course_dirs, # don't allow metadata on verticals, since we can't edit them in studio err_cnt += validate_no_non_editable_metadata(module_store, course_id, "vertical") # don't allow metadata on chapters, since we can't edit them in studio - err_cnt += validate_no_non_editable_metadata(module_store, course_id, "chapter",['start']) + err_cnt += validate_no_non_editable_metadata(module_store, course_id, "chapter") # don't allow metadata on sequences that we can't edit - err_cnt += validate_no_non_editable_metadata(module_store, course_id, "sequential", - ['due','format','start','graded']) + err_cnt += validate_no_non_editable_metadata(module_store, course_id, "sequential") # check for a presence of a course marketing video location_elements = course_id.split('/') From c7061a0262392c6e75747fcfed02dde0583c18c7 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 4 Apr 2013 10:20:31 -0400 Subject: [PATCH 2/5] add a command line utility to walk through the course and find common errors --- .../contentstore/management/commands/check_course.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/check_course.py b/cms/djangoapps/contentstore/management/commands/check_course.py index c691ac63a3..174048f108 100644 --- a/cms/djangoapps/contentstore/management/commands/check_course.py +++ b/cms/djangoapps/contentstore/management/commands/check_course.py @@ -19,12 +19,12 @@ class Command(BaseCommand): loc = CourseDescriptor.id_to_location(loc_str) - ms = modulestore() + modulestore = modulestore() # setup a request cache so we don't throttle the DB with all the metadata inheritance requests - ms.request_cache = RequestCache.get_request_cache() + modulestore.request_cache = RequestCache.get_request_cache() - course = ms.get_item(loc, depth=3) + course = modulestore.get_item(loc, depth=3) err_cnt = 0 def _xlint_metadata(module): From c0fc029b03a2eb8f01c186431366d5a2318cf498 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 4 Apr 2013 10:33:30 -0400 Subject: [PATCH 3/5] switch over the variable 'ms' to be 'store' --- .../contentstore/management/commands/check_course.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/check_course.py b/cms/djangoapps/contentstore/management/commands/check_course.py index 174048f108..35a11d7041 100644 --- a/cms/djangoapps/contentstore/management/commands/check_course.py +++ b/cms/djangoapps/contentstore/management/commands/check_course.py @@ -19,12 +19,12 @@ class Command(BaseCommand): loc = CourseDescriptor.id_to_location(loc_str) - modulestore = modulestore() + store = modulestore() # setup a request cache so we don't throttle the DB with all the metadata inheritance requests - modulestore.request_cache = RequestCache.get_request_cache() + store.request_cache = RequestCache.get_request_cache() - course = modulestore.get_item(loc, depth=3) + course = store.get_item(loc, depth=3) err_cnt = 0 def _xlint_metadata(module): @@ -61,7 +61,7 @@ class Command(BaseCommand): discussion_items =_get_discussion_items(course) # now query all discussion items via get_items() and compare with the tree-traversal - queried_discussion_items = ms.get_items(['i4x', course.location.org, course.location.course, + queried_discussion_items = store.get_items(['i4x', course.location.org, course.location.course, 'discussion', None, None]) for item in queried_discussion_items: From 73113eddc725bb05102e8b8f95b874f5b1e56d04 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 4 Apr 2013 13:38:30 -0400 Subject: [PATCH 4/5] fix xml_attributes inspection --- .../contentstore/management/commands/check_course.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/check_course.py b/cms/djangoapps/contentstore/management/commands/check_course.py index 35a11d7041..aec1a34c04 100644 --- a/cms/djangoapps/contentstore/management/commands/check_course.py +++ b/cms/djangoapps/contentstore/management/commands/check_course.py @@ -38,9 +38,10 @@ class Command(BaseCommand): # we've had a bug where the xml_attributes field can we rewritten as a string rather than a dict def _check_xml_attributes_field(module): err_cnt = 0 - if hasattr(module, 'xml_attributes') and (isinstance(module, str) or isinstance(module, unicode)): - print 'module = {0} has xml_attributes as a string. It should be a dict'.format(module.location.url()) - err_cnt = err_cnt + 1 + if hasattr(module, 'xml_attributes'): + if isinstance(module.xml_attributes, str) or isinstance(module.xml_attributes, unicode): + print 'module = {0} has xml_attributes as a string. It should be a dict'.format(module.location.url()) + err_cnt = err_cnt + 1 for child in module.get_children(): err_cnt = err_cnt + _check_xml_attributes_field(child) return err_cnt From 8e36197cc204de844de5809e542e6de2a7e26694 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Fri, 5 Apr 2013 09:30:49 -0400 Subject: [PATCH 5/5] tweeks per Cale's feedback --- .../contentstore/management/commands/check_course.py | 9 ++++----- common/lib/xmodule/xmodule/modulestore/xml_importer.py | 9 +++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/check_course.py b/cms/djangoapps/contentstore/management/commands/check_course.py index aec1a34c04..6d6c4ebde5 100644 --- a/cms/djangoapps/contentstore/management/commands/check_course.py +++ b/cms/djangoapps/contentstore/management/commands/check_course.py @@ -9,7 +9,7 @@ from request_cache.middleware import RequestCache from django.core.cache import get_cache, InvalidCacheBackendError class Command(BaseCommand): - help = '''Delete a MongoDB backed course''' + help = '''Enumerates through the course and find common errors''' def handle(self, *args, **options): if len(args) != 1 : @@ -38,10 +38,9 @@ class Command(BaseCommand): # we've had a bug where the xml_attributes field can we rewritten as a string rather than a dict def _check_xml_attributes_field(module): err_cnt = 0 - if hasattr(module, 'xml_attributes'): - if isinstance(module.xml_attributes, str) or isinstance(module.xml_attributes, unicode): - print 'module = {0} has xml_attributes as a string. It should be a dict'.format(module.location.url()) - err_cnt = err_cnt + 1 + if hasattr(module, 'xml_attributes') and isinstance(module.xml_attributes, basestring): + print 'module = {0} has xml_attributes as a string. It should be a dict'.format(module.location.url()) + err_cnt = err_cnt + 1 for child in module.get_children(): err_cnt = err_cnt + _check_xml_attributes_field(child) return err_cnt diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 0a7d07afe1..53eaebf850 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -379,10 +379,11 @@ def check_module_metadata_editability(module): allowed = allowed + ['xml_attributes', 'display_name'] err_cnt = 0 my_metadata = dict(own_metadata(module)) - for key in my_metadata.keys(): - if key not in allowed: - err_cnt = err_cnt + 1 - print ': found metadata on {0}. Studio will not support editing this piece of metadata, so it is not allowed. Metadata: {1} = {2}'. format(module.location.url(), key, my_metadata[key]) + illegal_keys = set(own_metadata(module).keys()) - set(allowed) + + if len(illegal_keys) > 0: + err_cnt = err_cnt + 1 + print ': found non-editable metadata on {0}. These metadata keys are not supported = {1}'. format(module.location.url(), illegal_keys) return err_cnt