Skip to content

Rewrite Reference and ReferenceList fields on import #2543

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
cahrens merged 1 commit into from
Feb 12, 2014
Merged

Rewrite Reference and ReferenceList fields on import #2543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ These are notable changes in edx-platform. This is a rolling list of changes,
in roughly chronological order, most recent first. Add your entries at or near
the top. Include a label indicating the component affected.

Studo: Fix import/export bug with conditional modules. STUD-149

Blades: Persist student progress in video. BLD-385.

Blades: Fix for the list metadata editor that gets into a bad state where "Add"
Expand Down
59 changes: 52 additions & 7 deletions ...ontentstore/tests/test_import_nostatic.py → ...ngoapps/contentstore/tests/test_import.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#pylint: disable=E1101
'''
Tests for importing with no static
'''
"""
Tests for import_from_xml using the mongo modulestore.
"""

from django.test.client import Client
from django.test.utils import override_settings
Expand Down Expand Up @@ -32,7 +32,7 @@


@override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE, MODULESTORE=TEST_MODULESTORE)
class ContentStoreImportNoStaticTest(ModuleStoreTestCase):
class ContentStoreImportTest(ModuleStoreTestCase):
"""
Tests that rely on the toy and test_import_course courses.
NOTE: refactor using CourseFactory so they do not.
Expand Down Expand Up @@ -130,7 +130,52 @@ def test_no_static_link_rewrites_on_import(self):
self.assertIn('/static/', handouts.data)

def test_tab_name_imports_correctly(self):
module_store, content_store, course, course_location = self.load_test_import_course()
_module_store, _content_store, course, _course_location = self.load_test_import_course()
print "course tabs = {0}".format(course.tabs)
self.assertEqual(course.tabs[2]['name'],'Syllabus')

self.assertEqual(course.tabs[2]['name'], 'Syllabus')

def test_rewrite_reference_list(self):
module_store = modulestore('direct')
target_location = Location(['i4x', 'testX', 'conditional_copy', 'course', 'copy_run'])
import_from_xml(
module_store,
'common/test/data/',
['conditional'],
target_location_namespace=target_location
)
conditional_module = module_store.get_item(
Location(['i4x', 'testX', 'conditional_copy', 'conditional', 'condone'])
)
self.assertIsNotNone(conditional_module)
self.assertListEqual(
[
u'i4x://testX/conditional_copy/problem/choiceprob',
u'i4x://edX/different_course/html/for_testing_import_rewrites'
],
conditional_module.sources_list
)
self.assertListEqual(
[
u'i4x://testX/conditional_copy/html/congrats',
u'i4x://testX/conditional_copy/html/secret_page'
],
conditional_module.show_tag_list
)

def test_rewrite_reference(self):
module_store = modulestore('direct')
target_location = Location(['i4x', 'testX', 'peergrading_copy', 'course', 'copy_run'])
import_from_xml(
module_store,
'common/test/data/',
['open_ended'],
target_location_namespace=target_location
)
peergrading_module = module_store.get_item(
Location(['i4x', 'testX', 'peergrading_copy', 'peergrading', 'PeerGradingLinked'])
)
self.assertIsNotNone(peergrading_module)
self.assertEqual(
u'i4x://testX/peergrading_copy/combinedopenended/SampleQuestion',
peergrading_module.link_to_location
)
65 changes: 43 additions & 22 deletions common/lib/xmodule/xmodule/modulestore/xml_importer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

from .xml import XMLModuleStore, ImportSystem, ParentTracker
from xmodule.modulestore import Location
from xblock.fields import Scope, Reference, ReferenceList
from xmodule.contentstore.content import StaticContent
from .inheritance import own_metadata
from xmodule.errortracker import make_error_tracker
Expand Down Expand Up @@ -424,7 +425,7 @@ def import_course_draft(
# aka sequential), so we have to replace the location.name
# with the XML filename that is part of the pack
fn, fileExtension = os.path.splitext(filename)
descriptor.location = descriptor.location._replace(name=fn)
descriptor.location = descriptor.location.replace(name=fn)

index = int(descriptor.xml_attributes['index_in_children_list'])
if index in drafts:
Expand All @@ -442,21 +443,21 @@ def import_course_draft(
for descriptor in drafts[key]:
try:
def _import_module(module):
module.location = module.location._replace(revision='draft')
module.location = module.location.replace(revision='draft')
# make sure our parent has us in its list of children
# this is to make sure private only verticals show up
# in the list of children since they would have been
# filtered out from the non-draft store export
if module.location.category == 'vertical':
non_draft_location = module.location._replace(revision=None)
non_draft_location = module.location.replace(revision=None)
sequential_url = module.xml_attributes['parent_sequential_url']
index = int(module.xml_attributes['index_in_children_list'])

seq_location = Location(sequential_url)

# IMPORTANT: Be sure to update the sequential
# in the NEW namespace
seq_location = seq_location._replace(
seq_location = seq_location.replace(
org=target_location_namespace.org,
course=target_location_namespace.course
)
Expand Down Expand Up @@ -486,20 +487,21 @@ def remap_namespace(module, target_location_namespace):
if target_location_namespace is None:
return module

original_location = module.location

# This looks a bit wonky as we need to also change the 'name' of the
# imported course to be what the caller passed in
if module.location.category != 'course':
module.location = module.location._replace(
module.location = module.location.replace(
tag=target_location_namespace.tag,
org=target_location_namespace.org,
course=target_location_namespace.course
)
else:
original_location = module.location
#
# module is a course module
#
module.location = module.location._replace(
module.location = module.location.replace(
tag=target_location_namespace.tag,
org=target_location_namespace.org,
course=target_location_namespace.course,
Expand All @@ -524,22 +526,41 @@ def remap_namespace(module, target_location_namespace):

module.save()

# then remap children pointers since they too will be re-namespaced
all_fields = module.get_explicitly_set_fields_by_scope(Scope.content)
all_fields.update(module.get_explicitly_set_fields_by_scope(Scope.settings))
if hasattr(module, 'children'):
children_locs = module.children
if children_locs is not None and children_locs != []:
new_locs = []
for child in children_locs:
child_loc = Location(child)
new_child_loc = child_loc._replace(
tag=target_location_namespace.tag,
org=target_location_namespace.org,
course=target_location_namespace.course
)

new_locs.append(new_child_loc.url())

module.children = new_locs
all_fields['children'] = module.children

def convert_ref(reference):
"""
Convert a reference to the new namespace, but only
if the original namespace matched the original course.

Otherwise, returns the input value.
"""
new_ref = reference
ref = Location(reference)
in_original_namespace = (original_location.tag == ref.tag and
original_location.org == ref.org and
original_location.course == ref.course)
if in_original_namespace:
new_ref = ref.replace(
tag=target_location_namespace.tag,
org=target_location_namespace.org,
course=target_location_namespace.course
).url()

@singingwolfboy singingwolfboy Feb 11, 2014

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.

The spacing makes this hard to read. I'd format this more like:

if (original_location.tag == ref.tag and 
        original_location.org == ref.org and
        original_location.course == ref.course):
    new_ref = ref.replace(
        tag=target_location_namespace.tag,
        org=target_location_namespace.org,
        course=target_location_namespace.course
    ).url()

or

def locations_equal(a, b):
    return (a.tag == b.tag and a.org == b.org and a.course == b.course)

if locations_equal(original_location, ref):
    new_ref = ref.replace(
        tag=target_location_namespace.tag,
        org=target_location_namespace.org,
        course=target_location_namespace.course
    ).url()

return new_ref

for field in all_fields:
if isinstance(module.fields.get(field), Reference):
new_ref = convert_ref(getattr(module, field))
setattr(module, field, new_ref)
module.save()
elif isinstance(module.fields.get(field), ReferenceList):

@singingwolfboy singingwolfboy Feb 11, 2014

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.

Using isinstance is sometimes necessary, but it isn't very Pythonic. Is there some way that you can check a property on the object for this test, instead of using isinstance? If the answer is "no", or "yes, but it would be very difficult and finicky", then isinstance is fine, but if the answer is "yes, and it's fairly straightforward", then do that, instead.

@cahrens cahrens Feb 11, 2014

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I don't think there is. The whole point of introducing Reference and ReferenceList was so that we could tell when an xblock Field was intended to hold references (locations or locators). The desire was to remove the guessing, which is admittedly not a Pythonic thing to do. :)

references = getattr(module, field)
new_references = [convert_ref(reference) for reference in references]
setattr(module, field, new_references)
module.save()

return module

Expand Down
8 changes: 7 additions & 1 deletion common/test/data/conditional/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
course for testing conditional module
Course for testing conditional module.

Note that 'i4x://edX/different_course/html/for_testing_import_rewrites' in sources
is only present for testing that import does NOT rewrite references to
courses that differ from the original course being imported.

It is not expected that i4x://edX/different_course/html/for_testing_import_rewrites will render.


2 changes: 1 addition & 1 deletion common/test/data/conditional/conditional/condone.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<conditional correct="True" sources="i4x://edX/cond_test/problem/choiceprob">
<conditional correct="True" sources="i4x://edX/cond_test/problem/choiceprob; i4x://edX/different_course/html/for_testing_import_rewrites">
<html>Conditionally shown page</html>
<show sources="i4x://edX/cond_test/html/congrats; i4x://edX/cond_test/html/secret_page"/>
</conditional>