Skip to content

Add options to dump_course_structure to output inherited metadata. #2008

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
brianhw merged 2 commits into from
Jan 3, 2014
Merged

Add options to dump_course_structure to output inherited metadata. #2008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4569ee9
Add options to dump_course_structure to output inherited metadata.
brianhw Dec 19, 2013
d0fea42
Add tests for inherited metadata to test_dump_course, and move to com…
brianhw Dec 30, 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
44 changes: 38 additions & 6 deletions lms/djangoapps/courseware/management/commands/dump_course_structure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,11 @@
from django.core.management.base import BaseCommand, CommandError

from xmodule.modulestore.django import modulestore
from xmodule.modulestore.inheritance import own_metadata

from xmodule.modulestore.inheritance import own_metadata, compute_inherited_metadata
from xblock.fields import Scope

FILTER_LIST = ['xml_attributes', 'checklists']
INHERITED_FILTER_LIST = ['children', 'xml_attributes', 'checklists']


class Command(BaseCommand):
Expand All @@ -41,6 +42,14 @@ class Command(BaseCommand):
action='store',
default='default',
help='Name of the modulestore'),
make_option('--inherited',
action='store_true',
default=False,
help='Whether to include inherited metadata'),
make_option('--inherited_defaults',
action='store_true',
default=False,
help='Whether to include default values of inherited metadata'),
)

def handle(self, *args, **options):
Expand All @@ -62,14 +71,18 @@ def handle(self, *args, **options):
if course is None:
raise CommandError("Invalid course_id")

# precompute inherited metadata at the course level, if needed:
Copy link
Contributor

@mulby mulby Jan 3, 2014

Choose a reason for hiding this comment

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

We can't do this inside dump_module?

Copy link
Contributor Author

@brianhw brianhw Jan 3, 2014

Choose a reason for hiding this comment

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

This is a one-time call -- it should be called only once for the entire course. dump_module is called recursively.

if options['inherited']:
compute_inherited_metadata(course)

# Convert course data to dictionary and dump it as JSON to stdout

info = dump_module(course)
info = dump_module(course, inherited=options['inherited'], defaults=options['inherited_defaults'])

return json.dumps(info, indent=2, sort_keys=True)


def dump_module(module, destination=None):
def dump_module(module, destination=None, inherited=False, defaults=False):
"""
Add the module and all its children to the destination dictionary in
as a flat structure.
Expand All @@ -83,10 +96,29 @@ def dump_module(module, destination=None):
destination[module.location.url()] = {
'category': module.location.category,
'children': module.children if hasattr(module, 'children') else [],
'metadata': filtered_metadata
'metadata': filtered_metadata,
}

if inherited:
# when calculating inherited metadata, don't include existing
# locally-defined metadata
inherited_metadata_filter_list = list(filtered_metadata.keys())
Copy link
Contributor

@mulby mulby Jan 3, 2014

Choose a reason for hiding this comment

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

[OPTIONAL]
I feel like a set might be more appropriate here:

inherited_metadata_filter = frozenset(filtered_metadata.keys() + INHERITED_FILTER_LIST)

inherited_metadata_filter_list.extend(INHERITED_FILTER_LIST)

def is_inherited(field):
if field.name in inherited_metadata_filter_list:
return False
elif field.scope != Scope.settings:
return False
elif defaults == True:
return True
else:
return field.values != field.default

inherited_metadata = {field.name: field.read_json(module) for field in module.fields.values() if is_inherited(field)}
Copy link
Contributor

@mulby mulby Jan 3, 2014

Choose a reason for hiding this comment

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

[OPTIONAL]
I personally prefer more verbose loops over (dict/list)-comprehensions because I find it easier to read/grok quickly. Personal preference though, so feel free to leave it as is.

inherited_metadata = {}
for field_name, field in module.fields.iteritems():  # this makes it more clear to me why you are only using the values
    if is_inherited(field):
        inherited_metadata[field_name] = field.read_json(module)

destination[module.location.url()]['inherited_metadata'] = inherited_metadata

for child in module.get_children():
dump_module(child, destination)
dump_module(child, destination, inherited, defaults)

return destination
0 ...re/management/tests/test_clean_history.py → ...ment/commands/tests/test_clean_history.py
View file
Open in desktop
File renamed without changes.
48 changes: 46 additions & 2 deletions ...ngoapps/courseware/tests/test_commands.py → ...gement/commands/tests/test_dump_course.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

from django.core.management import call_command
from django.test.utils import override_settings
from django.test.testcases import TestCase

from courseware.tests.modulestore_config import TEST_DATA_XML_MODULESTORE
from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
Expand All @@ -24,7 +25,7 @@
TEST_COURSE_ID = 'edX/simple/2012_Fall'


class CommandsTestBase(object):
class CommandsTestBase(TestCase):
"""
Base class for testing different django commands.

Expand Down Expand Up @@ -66,6 +67,15 @@ def test_dump_course_structure(self):

dump = json.loads(output)

# check that all elements in the course structure have metadata,
# but not inherited metadata:
for element_name in dump:
element = dump[element_name]
self.assertIn('metadata', element)
self.assertIn('children', element)
self.assertIn('category', element)
self.assertNotIn('inherited_metadata', element)

# Check a few elements in the course dump

parent_id = 'i4x://edX/simple/chapter/Overview'
Expand All @@ -81,10 +91,44 @@ def test_dump_course_structure(self):
self.assertEqual(len(dump[video_id]['metadata']), 4)
self.assertIn('youtube_id_1_0', dump[video_id]['metadata'])

# Check if there is the right number of elements
# Check if there are the right number of elements

self.assertEqual(len(dump), 16)

def test_dump_inherited_course_structure(self):
args = [TEST_COURSE_ID]
kwargs = {'modulestore': 'default', 'inherited': True}
output = self.call_command('dump_course_structure', *args, **kwargs)
dump = json.loads(output)
# check that all elements in the course structure have inherited metadata,
# and that it contains a particular value as well:
for element_name in dump:
element = dump[element_name]
self.assertIn('metadata', element)
self.assertIn('children', element)
self.assertIn('category', element)
self.assertIn('inherited_metadata', element)
self.assertIsNone(element['inherited_metadata']['ispublic'])
# ... but does not contain inherited metadata containing a default value:
self.assertNotIn('due', element['inherited_metadata'])

def test_dump_inherited_course_structure_with_defaults(self):
args = [TEST_COURSE_ID]
kwargs = {'modulestore': 'default', 'inherited': True, 'inherited_defaults': True}
output = self.call_command('dump_course_structure', *args, **kwargs)
dump = json.loads(output)
# check that all elements in the course structure have inherited metadata,
# and that it contains a particular value as well:
for element_name in dump:
element = dump[element_name]
self.assertIn('metadata', element)
self.assertIn('children', element)
self.assertIn('category', element)
self.assertIn('inherited_metadata', element)
self.assertIsNone(element['inherited_metadata']['ispublic'])
# ... and contains inherited metadata containing a default value:
self.assertIsNone(element['inherited_metadata']['due'])

def test_export_course(self):
tmp_dir = path(mkdtemp())
filename = tmp_dir / 'test.tar.gz'
Expand Down