Fixing github_sync to work with multiple course data directories

cms/djangoapps/contentstore/__init__.py

@@ -5,12 +5,17 @@ import logging
 log = logging.getLogger(__name__)
 
 
-def import_from_xml(org, course, data_dir):
+def import_from_xml(data_dir, course_dirs=None):
     """
     Import the specified xml data_dir into the django defined modulestore,
     using org and course as the location org and course.
     """
-    module_store = XMLModuleStore(org, course, data_dir, 'xmodule.raw_module.RawDescriptor', eager=True)
+    module_store = XMLModuleStore(
+        data_dir,
+        default_class='xmodule.raw_module.RawDescriptor',
+        eager=True,
+        course_dirs=course_dirs
+    )
     for module in module_store.modules.itervalues():
 
         # TODO (cpennington): This forces import to overrite the same items.
@@ -26,4 +31,4 @@ def import_from_xml(org, course, data_dir):
             modulestore().update_children(module.location, module.definition['children'])
         modulestore().update_metadata(module.location, dict(module.metadata))
 
-    return module_store.course
+    return module_store

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

@@ -13,8 +13,12 @@ class Command(BaseCommand):
 '''Import the specified data directory into the default ModuleStore'''
 
     def handle(self, *args, **options):
-        if len(args) != 3:
-            raise CommandError("import requires 3 arguments: <org> <course> <data directory>")
+        if len(args) == 0:
+            raise CommandError("import requires at least one argument: <data directory> [<course dir>...]")
 
-        org, course, data_dir = args
-        import_from_xml(org, course, data_dir)
+        data_dir = args[0]
+        if len(args) > 1:
+            course_dirs = args[1:]
+        else:
+            course_dirs = None
+        import_from_xml(data_dir, course_dirs)

cms/djangoapps/contentstore/views.py

@@ -6,7 +6,7 @@ from django_future.csrf import ensure_csrf_cookie
 from fs.osfs import OSFS
 from django.core.urlresolvers import reverse
 from xmodule.modulestore import Location
-from github_sync import repo_path_from_location, export_to_github
+from github_sync import export_to_github
 
 from mitxmako.shortcuts import render_to_response
 from xmodule.modulestore.django import modulestore
@@ -51,11 +51,12 @@ def save_item(request):
     modulestore().update_item(item_id, data)
 
     # Export the course back to github
+    # This uses wildcarding to find the course, which requires handling
+    # multiple courses returned, but there should only ever be one
     course_location = Location(item_id)._replace(category='course', name=None)
     courses = modulestore().get_items(course_location)
     for course in courses:
-        repo_path = repo_path_from_location(course.location)
-        export_to_github(course, repo_path, "CMS Edit")
+        export_to_github(course, "CMS Edit")
 
     return HttpResponse(json.dumps({}))
 

cms/djangoapps/github_sync/__init__.py

@@ -12,6 +12,7 @@ from .exceptions import GithubSyncError
 
 log = logging.getLogger(__name__)
 
+
 def import_from_github(repo_settings):
     """
     Imports data into the modulestore based on the XML stored on github
@@ -19,10 +20,9 @@ def import_from_github(repo_settings):
     repo_settings is a dictionary with the following keys:
         path: file system path to the local git repo
         branch: name of the branch to track on github
-        org: name of the organization to use in the imported course
-        course: name of the coures to use in the imported course
     """
     repo_path = repo_settings['path']
+    data_dir, course_dir = os.path.split(repo_path)
 
     if not os.path.isdir(repo_path):
         Repo.clone_from(repo_settings['origin'], repo_path)
@@ -34,18 +34,12 @@ def import_from_github(repo_settings):
     # Do a hard reset to the remote branch so that we have a clean import
     git_repo.git.checkout(repo_settings['branch'])
     git_repo.head.reset('origin/%s' % repo_settings['branch'], index=True, working_tree=True)
-
-    return git_repo.head.commit.hexsha, import_from_xml(repo_settings['org'], repo_settings['course'], repo_path)
-
-
-def repo_path_from_location(location):
-    location = Location(location)
-    for name, repo in settings.REPOS.items():
-        if repo['org'] == location.org and repo['course'] == location.course:
-            return repo['path']
+    module_store = import_from_xml(data_dir, course_dirs=[course_dir])
+    return git_repo.head.commit.hexsha, module_store.courses[course_dir]
 
 
-def export_to_github(course, repo_path, commit_message):
+def export_to_github(course, commit_message):
+    repo_path = settings.DATA_DIR / course.metadata.get('course_dir', course.location.course)
     fs = OSFS(repo_path)
     xml = course.export_to_xml(fs)
 

cms/djangoapps/github_sync/tests/__init__.py

 from django.test import TestCase
 from path import path
 import shutil
-from github_sync import import_from_github, export_to_github, repo_path_from_location
+from github_sync import import_from_github, export_to_github
 from git import Repo
 from django.conf import settings
 from xmodule.modulestore.django import modulestore
@@ -10,6 +10,7 @@ from override_settings import override_settings
 from github_sync.exceptions import GithubSyncError
 
 
+@override_settings(DATA_DIR=path('test_root'))
 class GithubSyncTestCase(TestCase):
 
     def setUp(self):
@@ -29,8 +30,6 @@ class GithubSyncTestCase(TestCase):
             'path': self.repo_dir,
             'origin': self.remote_dir,
             'branch': 'master',
-            'org': 'org',
-            'course': 'course'
         })
 
     def tearDown(self):
@@ -49,7 +48,7 @@ class GithubSyncTestCase(TestCase):
         """
         self.assertEquals('Toy Course', self.import_course.metadata['display_name'])
         self.assertIn(
-            Location('i4x://org/course/chapter/Overview'),
+            Location('i4x://edx/local_repo/chapter/Overview'),
             [child.location for child in self.import_course.get_children()])
         self.assertEquals(1, len(self.import_course.get_children()))
 
@@ -58,7 +57,7 @@ class GithubSyncTestCase(TestCase):
         """
         Test that with the GITHUB_PUSH feature disabled, no content is pushed to the remote
         """
-        export_to_github(self.import_course, self.repo_dir, 'Test no-push')
+        export_to_github(self.import_course, 'Test no-push')
         self.assertEquals(1, Repo(self.remote_dir).head.commit.count())
 
     @override_settings(MITX_FEATURES={'GITHUB_PUSH': True})
@@ -67,7 +66,7 @@ class GithubSyncTestCase(TestCase):
         Test that with GITHUB_PUSH enabled, content is pushed to the remote
         """
         self.import_course.metadata['display_name'] = 'Changed display name'
-        export_to_github(self.import_course, self.repo_dir, 'Test push')
+        export_to_github(self.import_course, 'Test push')
         self.assertEquals(2, Repo(self.remote_dir).head.commit.count())
 
     @override_settings(MITX_FEATURES={'GITHUB_PUSH': True})
@@ -80,17 +79,6 @@ class GithubSyncTestCase(TestCase):
         remote = Repo(self.remote_dir)
         remote.git.commit(allow_empty=True, m="Testing conflict commit")
 
-        self.assertRaises(GithubSyncError, export_to_github, self.import_course, self.repo_dir, 'Test push')
+        self.assertRaises(GithubSyncError, export_to_github, self.import_course, 'Test push')
         self.assertEquals(2, remote.head.reference.commit.count())
         self.assertEquals("Testing conflict commit\n", remote.head.reference.commit.message)
-
-
-@override_settings(REPOS={'namea': {'path': 'patha', 'org': 'orga', 'course': 'coursea'},
-                          'nameb': {'path': 'pathb', 'org': 'orgb', 'course': 'courseb'}})
-class RepoPathLookupTestCase(TestCase):
-    def test_successful_lookup(self):
-        self.assertEquals('patha', repo_path_from_location('i4x://orga/coursea/course/foo'))
-        self.assertEquals('pathb', repo_path_from_location('i4x://orgb/courseb/course/foo'))
-
-    def test_failed_lookup(self):
-        self.assertEquals(None, repo_path_from_location('i4x://c/c/course/foo'))

common/lib/xmodule/tests/__init__.py

@@ -437,9 +437,7 @@ class GraderTest(unittest.TestCase):
         self.assertEqual( len(graded['section_breakdown']), 0 )
         self.assertEqual( len(graded['grade_breakdown']), 0 )
 
-
-
-def test_graderFromConf(self):
+    def test_graderFromConf(self):
         
         #Confs always produce a graders.WeightedSubsectionsGrader, so we test this by repeating the test
         #in test_graders.WeightedSubsectionsGrader, but generate the graders with confs.
@@ -493,16 +491,16 @@ def test_graderFromConf(self):
 # Module progress tests
     
 class ProgressTest(unittest.TestCase):
-''' Test that basic Progress objects work.  A Progress represents a
-fraction between 0 and 1.
-'''
-not_started = Progress(0, 17)
-part_done = Progress(2, 6)
-half_done = Progress(3, 6)
-also_half_done = Progress(1, 2)
-done = Progress(7, 7)
-
-def test_create_object(self):
+    ''' Test that basic Progress objects work.  A Progress represents a
+    fraction between 0 and 1.
+    '''
+    not_started = Progress(0, 17)
+    part_done = Progress(2, 6)
+    half_done = Progress(3, 6)
+    also_half_done = Progress(1, 2)
+    done = Progress(7, 7)
+
+    def test_create_object(self):
         # These should work:
         p = Progress(0, 2)
         p = Progress(1, 2)
@@ -522,13 +520,13 @@ def test_create_object(self):
         # check complex numbers just for the heck of it :)
         self.assertRaises(TypeError, Progress, 2j, 3)
 
-def test_frac(self):
+    def test_frac(self):
         p = Progress(1, 2)
         (a, b) = p.frac()
         self.assertEqual(a, 1)
         self.assertEqual(b, 2)
 
-def test_percent(self):
+    def test_percent(self):
         self.assertEqual(self.not_started.percent(), 0)
         self.assertAlmostEqual(self.part_done.percent(), 33.33333333333333)
         self.assertEqual(self.half_done.percent(), 50)
@@ -536,14 +534,14 @@ def test_percent(self):
 
         self.assertEqual(self.half_done.percent(), self.also_half_done.percent())
 
-def test_started(self):
+    def test_started(self):
         self.assertFalse(self.not_started.started())
 
         self.assertTrue(self.part_done.started())
         self.assertTrue(self.half_done.started())
         self.assertTrue(self.done.started())
 
-def test_inprogress(self):
+    def test_inprogress(self):
         # only true if working on it
         self.assertFalse(self.done.inprogress())
         self.assertFalse(self.not_started.inprogress())
@@ -551,22 +549,22 @@ def test_inprogress(self):
         self.assertTrue(self.part_done.inprogress())
         self.assertTrue(self.half_done.inprogress())
 
-def test_done(self):
+    def test_done(self):
         self.assertTrue(self.done.done())
         self.assertFalse(self.half_done.done())
         self.assertFalse(self.not_started.done())
         
-def test_str(self):
+    def test_str(self):
         self.assertEqual(str(self.not_started), "0/17")
         self.assertEqual(str(self.part_done), "2/6")
         self.assertEqual(str(self.done), "7/7")
 
-def test_ternary_str(self):
+    def test_ternary_str(self):
         self.assertEqual(self.not_started.ternary_str(), "none")
         self.assertEqual(self.half_done.ternary_str(), "in_progress")
         self.assertEqual(self.done.ternary_str(), "done")
 
-def test_to_js_status(self):
+    def test_to_js_status(self):
         '''Test the Progress.to_js_status_str() method'''
         
         self.assertEqual(Progress.to_js_status_str(self.not_started), "none")
@@ -574,7 +572,7 @@ def test_to_js_status(self):
         self.assertEqual(Progress.to_js_status_str(self.done), "done")
         self.assertEqual(Progress.to_js_status_str(None), "NA")
 
-def test_to_js_detail_str(self):
+    def test_to_js_detail_str(self):
         '''Test the Progress.to_js_detail_str() method'''
         f = Progress.to_js_detail_str
         for p in (self.not_started, self.half_done, self.done):
@@ -582,7 +580,7 @@ def test_to_js_detail_str(self):
         # But None should be encoded as NA
         self.assertEqual(f(None), "NA")
 
-def test_add(self):
+    def test_add(self):
         '''Test the Progress.add_counts() method'''
         p = Progress(0, 2)
         p2 = Progress(1, 3)
@@ -597,7 +595,7 @@ def test_add(self):
         self.assertEqual(add(p2, pNone), p2.frac())
         self.assertEqual(add(pNone, p2), p2.frac())
 
-def test_equality(self):
+    def test_equality(self):
         '''Test that comparing Progress objects for equality
         works correctly.'''
         p = Progress(1, 2)
@@ -612,9 +610,9 @@ def test_equality(self):
 
 
 class ModuleProgressTest(unittest.TestCase):
-''' Test that get_progress() does the right thing for the different modules
-'''
-def test_xmodule_default(self):
+    ''' Test that get_progress() does the right thing for the different modules
+    '''
+    def test_xmodule_default(self):
         '''Make sure default get_progress exists, returns None'''
         xm = x_module.XModule(i4xs, 'a://b/c/d/e', {})
         p = xm.get_progress()

common/lib/xmodule/xmodule/capa_module.py

@@ -15,7 +15,6 @@ from xmodule.raw_module import RawDescriptor
 from progress import Progress
 from capa.capa_problem import LoncapaProblem
 from capa.responsetypes import StudentInputError
-from static_replace import replace_urls
 
 log = logging.getLogger("mitx.courseware")
 
@@ -275,7 +274,7 @@ class CapaModule(XModule):
             html = '<div id="problem_{id}" class="problem" data-url="{ajax_url}">'.format(
                 id=self.location.html_id(), ajax_url=self.system.ajax_url) + html + "</div>"
 
-        return replace_urls(html, self.metadata['data_dir'])
+        return self.system.replace_urls(html, self.metadata['data_dir'])
 
     def handle_ajax(self, dispatch, get):
         '''

common/lib/xmodule/xmodule/modulestore/xml.py

@@ -20,19 +20,21 @@ class XMLModuleStore(ModuleStore):
     """
     An XML backed ModuleStore
     """
-    def __init__(self, data_dir, default_class=None, eager=False):
+    def __init__(self, data_dir, default_class=None, eager=False, course_dirs=None):
         """
         Initialize an XMLModuleStore from data_dir
 
         data_dir: path to data directory containing the course directories
         default_class: dot-separated string defining the default descriptor class to use if non is specified in entry_points
         eager: If true, load the modules children immediately to force the entire course tree to be parsed
+        course_dirs: If specified, the list of course_dirs to load. Otherwise, load
+            all course dirs
         """
 
         self.eager = eager
         self.data_dir = path(data_dir)
         self.modules = {}
-        self.courses = []
+        self.courses = {}
 
         if default_class is None:
             self.default_class = None
@@ -46,10 +48,14 @@ class XMLModuleStore(ModuleStore):
         log.debug('default_class = %s' % self.default_class)
 
         for course_dir in os.listdir(self.data_dir):
-            if not os.path.exists(self.data_dir + "/" + course_dir + "/course.xml"):
+            if course_dirs is not None and course_dir not in course_dirs:
                 continue
 
-            self.courses.append(self.load_course(course_dir))
+            if not os.path.exists(self.data_dir / course_dir / "course.xml"):
+                continue
+
+            course_descriptor = self.load_course(course_dir)
+            self.courses[course_dir] = course_descriptor
 
     def load_course(self, course_dir):
         """
@@ -148,7 +154,7 @@ class XMLModuleStore(ModuleStore):
         """
         Returns a list of course descriptors
         """
-        return self.courses
+        return self.courses.values()
 
     def create_item(self, location):
         raise NotImplementedError("XMLModuleStores are read-only")

lms/djangoapps/courseware/module_render.py

@@ -27,8 +27,8 @@ class I4xSystem(object):
     and user, or other environment-specific info.
     '''
     def __init__(self, ajax_url, track_function,
-                 get_module, render_template, user=None,
-                 filestore=None):
+                 get_module, render_template, replace_urls,
+                 user=None, filestore=None):
         '''
         Create a closure around the system environment.
 
@@ -44,6 +44,8 @@ class I4xSystem(object):
         user - The user to base the seed off of for this request
         filestore - A filestore ojbect.  Defaults to an instance of OSFS based at
                     settings.DATA_DIR.
+        replace_urls - TEMPORARY - A function like static_replace.replace_urls
+            that capa_module can use to fix up the static urls in ajax results.
         '''
         self.ajax_url = ajax_url
         self.track_function = track_function
@@ -53,6 +55,7 @@ class I4xSystem(object):
         self.exception404 = Http404
         self.DEBUG = settings.DEBUG
         self.seed = user.id if user is not None else 0
+        self.replace_urls = replace_urls
 
     def get(self, attr):
         '''	provide uniform access to attributes (like etree).'''
@@ -209,6 +212,9 @@ def get_module(user, request, location, student_module_cache, position=None):
         (module, _, _, _) = get_module(user, request, location, student_module_cache, position)
         return module
 
+    # 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 = I4xSystem(track_function=make_track_function(request),
                        render_template=render_to_string,
                        ajax_url=ajax_url,
@@ -216,17 +222,18 @@ def get_module(user, request, location, student_module_cache, position=None):
                        filestore=descriptor.system.resources_fs,
                        get_module=_get_module,
                        user=user,
+                       # TODO (cpennington): This should be removed when all html from
+                       # a module is coming through get_html and is therefore covered
+                       # by the replace_static_urls code below
+                       replace_urls=replace_urls,
                        )
     # pass position specified in URL to module through I4xSystem
     system.set('position', position)
 
     module = descriptor.xmodule_constructor(system)(instance_state, shared_state)
 
-    # 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
-    prefix = module.metadata['data_dir']
-    module = replace_static_urls(module, prefix)
+    replace_prefix = module.metadata['data_dir']
+    module = replace_static_urls(module, replace_prefix)
 
     if settings.MITX_FEATURES.get('DISPLAY_HISTOGRAMS_TO_STAFF') and user.is_staff:
         module = add_histogram(module)