Review-driven changes (to-be-squashed)

cms/djangoapps/contentstore/tests/test_course_settings.py

@@ -120,8 +120,8 @@ class CourseDetailsTestCase(CourseTestCase):
 
             self.assertContains(response, "Introducing Your Course")
             self.assertContains(response, "Course Image")
-            self.assertNotContains(response,"Course Overview")
-            self.assertNotContains(response,"Course Introduction Video")
+            self.assertNotContains(response, "Course Overview")
+            self.assertNotContains(response, "Course Introduction Video")
             self.assertNotContains(response, "Requirements")
 
     def test_regular_site_fetch(self):
@@ -141,8 +141,8 @@ class CourseDetailsTestCase(CourseTestCase):
 
             self.assertContains(response, "Introducing Your Course")
             self.assertContains(response, "Course Image")
-            self.assertContains(response,"Course Overview")
-            self.assertContains(response,"Course Introduction Video")
+            self.assertContains(response, "Course Overview")
+            self.assertContains(response, "Course Introduction Video")
             self.assertContains(response, "Requirements")
 
 
@@ -259,7 +259,8 @@ class CourseGradingTest(CourseTestCase):
     def test_update_grader_from_json(self):
         test_grader = CourseGradingModel.fetch(self.course_locator)
         altered_grader = CourseGradingModel.update_grader_from_json(
-            self.course_locator, test_grader.graders[1], self.user)
+            self.course_locator, test_grader.graders[1], self.user
+        )
         self.assertDictEqual(test_grader.graders[1], altered_grader, "Noop update")
 
         test_grader.graders[1]['min_count'] = test_grader.graders[1].get('min_count') + 2
@@ -293,7 +294,8 @@ class CourseGradingTest(CourseTestCase):
     def test_delete_grace_period(self):
         test_grader = CourseGradingModel.fetch(self.course_locator)
         CourseGradingModel.update_grace_period_from_json(
-            self.course_locator, test_grader.grace_period, self.user)
+            self.course_locator, test_grader.grace_period, self.user
+        )
         # update_grace_period_from_json doesn't return anything, so query the db for its contents.
         altered_grader = CourseGradingModel.fetch(self.course_locator)
         self.assertEqual(test_grader.grace_period, altered_grader.grace_period, "Noop update")
@@ -445,9 +447,10 @@ class CourseMetadataEditingTest(CourseTestCase):
 
     def test_update_from_json(self):
         test_model = CourseMetadata.update_from_json(
-            self.course, {
+            self.course,
+            {
                 "advertised_start": "start A",
-                "days_early_for_beta": 2
+                "days_early_for_beta": 2,
             },
             user=self.user
          )
@@ -458,9 +461,10 @@ class CourseMetadataEditingTest(CourseTestCase):
         self.update_check(test_model)
         # now change some of the existing metadata
         test_model = CourseMetadata.update_from_json(
-            fresh, {
+            fresh,
+            {
                 "advertised_start": "start B",
-                "display_name": "jolly roger"
+                "display_name": "jolly roger",
             },
             user=self.user
         )
@@ -479,9 +483,8 @@ class CourseMetadataEditingTest(CourseTestCase):
 
     def test_delete_key(self):
         test_model = CourseMetadata.update_from_json(
-            self.fullcourse, {
-                "unsetKeys": ['showanswer', 'xqa_key']
-            },
+            self.fullcourse,
+            {"unsetKeys": ['showanswer', 'xqa_key']},
             user=self.user
         )
         # ensure no harm

cms/djangoapps/contentstore/views/course.py

@@ -625,7 +625,7 @@ def advanced_settings_handler(request, package_id=None, branch=None, version_gui
                     course_module,
                     request.json,
                     filter_tabs=filter_tabs,
-                    user=request.user
+                    user=request.user,
                 ))
             except (TypeError, ValueError) as err:
                 return HttpResponseBadRequest(

cms/djangoapps/contentstore/views/item.py

@@ -9,7 +9,6 @@ from functools import partial
 from static_replace import replace_static_urls
 from xmodule_modifiers import wrap_xblock
 
-from django.conf import settings
 from django.core.exceptions import PermissionDenied
 from django.contrib.auth.decorators import login_required
 from django.http import HttpResponseBadRequest, HttpResponse
@@ -18,9 +17,8 @@ from django.views.decorators.http import require_http_methods
 
 from xblock.fields import Scope
 from xblock.fragment import Fragment
-from xblock.core import XBlock
 
-import xmodule.x_module
+import xmodule
 from xmodule.modulestore.django import modulestore, loc_mapper
 from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError
 from xmodule.modulestore.inheritance import own_metadata
@@ -185,7 +183,7 @@ def xblock_handler(request, tag=None, package_id=None, branch=None, version_guid
                 parent_location,
                 duplicate_source_location,
                 request.json.get('display_name'),
-                request.user
+                request.user,
             )
             course_location = loc_mapper().translate_locator_to_location(BlockUsageLocator(parent_locator), get_course=True)
             dest_locator = loc_mapper().translate_location(course_location.course_id, dest_location, False, True)
@@ -379,7 +377,8 @@ def _duplicate_item(parent_location, duplicate_source_location, display_name=Non
     if source_item.has_children:
         dest_module.children = []
         for child in source_item.children:
-            dest_module.children.append(_duplicate_item(dest_location, Location(child), user=user).url())
+            dupe = _duplicate_item(dest_location, Location(child), user=user)
+            dest_module.children.append(dupe.url())
         get_modulestore(dest_location).update_item(dest_module, user.id if user else None)
 
     if not 'detached' in source_item.runtime.load_block_type(category)._class_tags:

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -418,15 +418,13 @@ class ModuleStoreWrite(ModuleStoreRead):
         Update the given xblock's persisted repr. Pass the user's unique id which the persistent store
         should save with the update if it has that ability.
 
-        :param allow_not_found: whether this method should raise an exception of the given xblock
+        :param allow_not_found: whether this method should raise an exception if the given xblock
         has not been persisted before.
-
-        For version tracking and conflict detecting persistence stores
+        :param force: fork the structure and don't update the course draftVersion if there's a version
+        conflict (only applicable to version tracking and conflict detecting persistence stores)
 
         :raises VersionConflictError: if package_id and version_guid given and the current
-        version head != version_guid and force is not True.
-        :param force: fork the structure and don't update the course draftVersion if the above
-
+        version head != version_guid and force is not True. (only applicable to version tracking stores)
         """
         pass
 
@@ -438,12 +436,11 @@ class ModuleStoreWrite(ModuleStoreRead):
 
         :param delete_all_versions: removes both the draft and published version of this item from
         the course if using draft and old mongo. Split may or may not implement this.
-
-        For version tracking and conflict detecting persistence stores
+        :param force: fork the structure and don't update the course draftVersion if there's a version
+        conflict (only applicable to version tracking and conflict detecting persistence stores)
 
         :raises VersionConflictError: if package_id and version_guid given and the current
-        version head != version_guid and force is not True.
-        :param force: fork the structure and don't update the course draftVersion if the above
+        version head != version_guid and force is not True. (only applicable to version tracking stores)
         """
         pass
 

common/lib/xmodule/xmodule/modulestore/mixed.py

@@ -59,67 +59,81 @@ class MixedModuleStore(ModuleStoreWriteBase):
         mapping = self.mappings.get(course_id, 'default')
         return self.modulestores[mapping]
 
-    def _incoming_reference_adaptor(self, store, course_id, reference):
+    def _locator_to_location(self, reference):
         """
-        Convert the reference to the type the persistence layer wants
+        Convert the referenced locator to a location casting to and from a string as necessary
+        """
+        stringify = isinstance(reference, basestring)
+        if stringify:
+            reference = BlockUsageLocator(url=reference)
+        location = loc_mapper().translate_locator_to_location(reference)
+        return location.url() if stringify else location
+
+    def _location_to_locator(self, course_id, reference):
+        """
+        Convert the referenced location to a locator casting to and from a string as necessary
         """
-        if issubclass(store.reference_type, Location if self.use_locations else Locator):
-            return reference
         stringify = isinstance(reference, basestring)
-        if store.reference_type == Location:
-            if stringify:
-                reference = BlockUsageLocator(url=reference)
-            location = loc_mapper().translate_locator_to_location(reference)
-            return location.url() if stringify else location
         if stringify:
             reference = Location(reference)
         locator = loc_mapper().translate_location(course_id, reference, reference.revision == 'draft', True)
         return unicode(locator) if stringify else locator
 
+    def _incoming_reference_adaptor(self, store, course_id, reference):
+        """
+        Convert the reference to the type the persistence layer wants
+        """
+        if issubclass(store.reference_type, Location if self.use_locations else Locator):
+            return reference
+        if store.reference_type == Location:
+            return self._locator_to_location(reference)
+        return self._location_to_locator(course_id, reference)
+
     def _outgoing_reference_adaptor(self, store, course_id, reference):
         """
         Convert the reference to the type the application wants
         """
         if issubclass(store.reference_type, Location if self.use_locations else Locator):
             return reference
-        stringify = isinstance(reference, basestring)
         if store.reference_type == Location:
-            if stringify:
-                reference = Location(reference)
-            locator = loc_mapper().translate_location(
-                course_id, reference, reference.revision == 'draft', True
-            )
-            return unicode(locator) if stringify else locator
-        else:
-            if stringify:
-                reference = BlockUsageLocator(url=reference)
-            location = loc_mapper().translate_locator_to_location(reference)
-            return location.url() if stringify else location
+            return self._location_to_locator(course_id, reference)
+        return self._locator_to_location(reference)
 
-    def _incoming_xblock_adaptor(self, store, course_id, xblock):
+    def _xblock_adaptor_iterator(self, adaptor, string_converter, store, course_id, xblock):
         """
-        Change all reference fields in this xblock to the type expected by the persistence layer
+        Change all reference fields in this xblock to the type expected by the receiving layer
         """
-        string_converter = self._get_string_converter(
-            course_id, store.reference_type, xblock.location
-        )
         for field in xblock.fields.itervalues():
             if field.is_set_on(xblock):
                 if isinstance(field, Reference):
                     field.write_to(
                         xblock,
-                        self._incoming_reference_adaptor(store, course_id, field.read_from(xblock)))
+                        adaptor(store, course_id, field.read_from(xblock))
+                    )
                 elif isinstance(field, ReferenceList):
                     field.write_to(
                         xblock,
-                        [self._incoming_reference_adaptor(store, course_id, ref)
-                         for ref in field.read_from(xblock)]
+                        [
+                            adaptor(store, course_id, ref)
+                            for ref in field.read_from(xblock)
+                        ]
                     )
-                elif string_converter is not None and isinstance(field, String):
+                elif isinstance(field, String):
                     # replace links within the string
                     string_converter(field, xblock)
         return xblock
 
+    def _incoming_xblock_adaptor(self, store, course_id, xblock):
+        """
+        Change all reference fields in this xblock to the type expected by the persistence layer
+        """
+        string_converter = self._get_string_converter(
+            course_id, store.reference_type, xblock.location
+        )
+        return self._xblock_adaptor_iterator(
+            self._incoming_reference_adaptor, string_converter, store, course_id, xblock
+        )
+
     def _outgoing_xblock_adaptor(self, store, course_id, xblock):
         """
         Change all reference fields in this xblock to the type expected by the persistence layer
@@ -127,22 +141,9 @@ class MixedModuleStore(ModuleStoreWriteBase):
         string_converter = self._get_string_converter(
             course_id, xblock.location.__class__, xblock.location
         )
-        for field in xblock.fields.itervalues():
-            if field.is_set_on(xblock):
-                if isinstance(field, Reference):
-                    field.write_to(
-                        xblock,
-                        self._outgoing_reference_adaptor(store, course_id, field.read_from(xblock)))
-                elif isinstance(field, ReferenceList):
-                    field.write_to(
-                        xblock,
-                        [self._outgoing_reference_adaptor(store, course_id, ref)
-                         for ref in field.read_from(xblock)]
-                    )
-                elif string_converter is not None and isinstance(field, String):
-                    # replace links within the string
-                    string_converter(field, xblock)
-        return xblock
+        return self._xblock_adaptor_iterator(
+            self._outgoing_reference_adaptor, string_converter, store, course_id, xblock
+        )
 
     CONVERT_RE = re.compile(r"/jump_to_id/({}+)".format(ALLOWED_ID_CHARS))
 
@@ -152,9 +153,9 @@ class MixedModuleStore(ModuleStoreWriteBase):
         with the correct rewritten link for the target type
         """
         if self.use_locations and reference_type == Location:
-            return None
+            return lambda field, xblock: None
         if not self.use_locations and issubclass(reference_type, Locator):
-            return None
+            return lambda field, xblock: None
         if isinstance(from_base_addr, Location):
             def mapper(found_id):
                 """

common/lib/xmodule/xmodule/modulestore/mongo/base.py

@@ -223,7 +223,6 @@ def namedtuple_to_son(namedtuple, prefix=''):
     # pylint: disable=protected-access
     for idx, field_name in enumerate(namedtuple._fields):
         son[prefix + field_name] = namedtuple[idx]
-    # pylint: enable=protected-access
     return son
 
 
@@ -256,6 +255,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
     """
     A Mongodb backed ModuleStore
     """
+    reference_type = Location
 
     # TODO (cpennington): Enable non-filesystem filestores
     # pylint: disable=C0103
@@ -313,7 +313,6 @@ class MongoModuleStore(ModuleStoreWriteBase):
         self.error_tracker = error_tracker
         self.render_template = render_template
         self.ignore_write_events_on_courses = []
-        self.reference_type = Location
 
     def compute_metadata_inheritance_tree(self, location):
         '''
@@ -794,17 +793,17 @@ class MongoModuleStore(ModuleStoreWriteBase):
                                    for child in xblock.children]
                 payload.update({'definition.children': xblock.children})
             self._update_single_item(xblock.location, payload)
+            # for static tabs, their containing course also records their display name
             if xblock.category == 'static_tab':
                 course = self._get_course_for_item(xblock.location)
-                existing_tabs = course.tabs or []
-                for tab in existing_tabs:
+                # find the course's reference to this tab and update the name.
+                for tab in course.tabs:
                     if tab.get('url_slug') == xblock.location.name:
-                        if xblock.fields['display_name'].is_set_on(xblock):
+                        # only update if changed
+                        if tab['name'] != xblock.display_name:
                             tab['name'] = xblock.display_name
-                        break
-                course.tabs = existing_tabs
-                # Save the updates to the course to the MongoKeyValueStore
-                self.update_item(course, user)
+                            self.update_item(course, user)
+                            break
 
             # recompute (and update) the metadata inheritance tree which is cached
             # was conditional on children or metadata having changed before dhm made one update to rule them all
@@ -831,8 +830,6 @@ class MongoModuleStore(ModuleStoreWriteBase):
             course = self._get_course_for_item(item.location)
             existing_tabs = course.tabs or []
             course.tabs = [tab for tab in existing_tabs if tab.get('url_slug') != location.name]
-            # Save the updates to the course to the MongoKeyValueStore
-            course.save()
             self.update_item(course, '**replace_user**')
 
         # Must include this to avoid the django debug toolbar (which defines the deprecated "safe=False")

common/lib/xmodule/xmodule/modulestore/mongo/draft.py

@@ -168,9 +168,9 @@ class DraftModuleStore(MongoModuleStore):
         try:
             if not self.has_item(None, draft_loc):
                 self.convert_to_draft(xblock.location)
-        except ItemNotFoundError, e:
+        except ItemNotFoundError:
             if not allow_not_found:
-                raise e
+                raise
 
         xblock.location = draft_loc
         super(DraftModuleStore, self).update_item(xblock, user)

common/lib/xmodule/xmodule/modulestore/split_mongo/split.py

@@ -100,6 +100,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
     A Mongodb backed ModuleStore supporting versions, inheritance,
     and sharing.
     """
+    reference_type = Locator
     def __init__(self, doc_store_config, fs_root, render_template,
                  default_class=None,
                  error_tracker=null_error_tracker,
@@ -111,7 +112,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
 
         super(SplitMongoModuleStore, self).__init__(**kwargs)
         self.loc_mapper = loc_mapper
-        self.reference_type = Locator
 
         self.db_connection = MongoConnection(**doc_store_config)
         self.db = self.db_connection.database

common/lib/xmodule/xmodule/modulestore/store_utilities.py

@@ -126,7 +126,8 @@ def _clone_modules(modulestore, modules, source_location, dest_location):
 
         if 'data' in module.fields and module.fields['data'].is_set_on(module) and isinstance(module.data, basestring):
             module.data = rewrite_nonportable_content_links(
-                source_location.course_id, dest_location.course_id, module.data)
+                source_location.course_id, dest_location.course_id, module.data
+            )
 
         # repoint children
         if module.has_children:

common/lib/xmodule/xmodule/modulestore/tests/test_locators.py

@@ -261,7 +261,7 @@ class LocatorTest(TestCase):
 
     def test_relative(self):
         """
-        It seems we used to use colons in names; so, ensure they're acceptable.
+        Test making a relative usage locator.
         """
         package_id = 'mit.eecs-1'
         branch = 'foo'

lms/djangoapps/courseware/tests/test_view_authentication.py

@@ -85,7 +85,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         urls.extend([
             reverse('book', kwargs={'course_id': course.id,
                                     'book_index': index})
-            for index, _book in enumerate(course.textbooks)
+            for index in xrange(len(course.textbooks))
         ])
         for url in urls:
             check_for_get_code(self, 200, url)