Merge pull request #1027 from MITx/origin/feature/cdodge/static-tab-edit

address feedback from cale

cms/djangoapps/contentstore/utils.py

@@ -33,31 +33,6 @@ def get_course_location_for_item(location):
 
     return location
 
-def get_course_for_item(location):
-    '''
-    cdodge: for a given Xmodule, return the course that it belongs to
-    NOTE: This makes a lot of assumptions about the format of the course location
-    Also we have to assert that this module maps to only one course item - it'll throw an
-    assert if not
-    '''
-    item_loc = Location(location)
-
-    # @hack! We need to find the course location however, we don't
-    # know the 'name' parameter in this context, so we have
-    # to assume there's only one item in this query even though we are not specifying a name
-    course_search_location = ['i4x', item_loc.org, item_loc.course, 'course', None]
-    courses = modulestore().get_items(course_search_location)
-
-    # make sure we found exactly one match on this above course search
-    found_cnt = len(courses)
-    if found_cnt == 0:
-        raise BaseException('Could not find course at {0}'.format(course_search_location))
-
-    if found_cnt > 1:
-        raise BaseException('Found more than one course at {0}. There should only be one!!! Dump = {1}'.format(course_search_location, courses))
-
-    return courses[0]
-
 
 def get_lms_link_for_item(location, preview=False):
     location = Location(location)

cms/djangoapps/contentstore/views.py

@@ -55,7 +55,7 @@ from cache_toolbox.core import set_cached_content, get_cached_content, del_cache
 from auth.authz import is_user_in_course_group_role, get_users_in_course_group_by_role
 from auth.authz import get_user_by_email, add_user_to_course_group, remove_user_from_course_group
 from auth.authz import INSTRUCTOR_ROLE_NAME, STAFF_ROLE_NAME, create_all_course_groups
-from .utils import get_course_location_for_item, get_lms_link_for_item, compute_unit_state, get_date_display, UnitState, get_course_for_item
+from .utils import get_course_location_for_item, get_lms_link_for_item, compute_unit_state, get_date_display, UnitState
 
 from xmodule.templates import all_templates
 from xmodule.modulestore.xml_importer import import_from_xml
@@ -622,17 +622,6 @@ def save_item(request):
         # commit to datastore
         store.update_metadata(item_location, existing_item.metadata)
 
-        # cdodge: special case logic for updating static_tabs
-        # unfortunately tabs are enumerated in the course policy data structure, so if we change the display name of
-        # the tab, we need to update the course policy (which has a nice .tabs property on it)
-        item_loc = Location(item_location)
-        if item_loc.category == 'static_tab':
-            # VS[compat] Rework when we can stop having to support tabs lists in the policy
-            tag_module = store.get_item(item_location)
-            course = get_course_for_item(item_location)
-            course.update_tab_reference(tag_module)
-            modulestore('direct').update_metadata(course.location, course.metadata)
-
     return HttpResponse()
 
 
@@ -709,13 +698,6 @@ def clone_item(request):
 
     if new_item.location.category not in DETACHED_CATEGORIES:
         _modulestore(parent.location).update_children(parent_location, parent.definition.get('children', []) + [new_item.location.url()])
-    elif new_item.location.category == 'static_tab':
-        # static tabs - in our data model - are described in the course policy
-        # VS[compat]: Rework when we can stop having to support tabs lists in the policy
-        if parent.location.category != 'course':
-            raise BaseException('adding a new static_tab must be on a course object')
-        parent.add_tab_reference(new_item)
-        _modulestore(parent.location).update_metadata(parent.location.url(), parent.metadata)
 
     return HttpResponse(json.dumps({'id': dest_location.url()}))
 

common/lib/xmodule/xmodule/course_module.py

@@ -270,29 +270,6 @@ class CourseDescriptor(SequenceDescriptor):
     def tabs(self, value):
         self.metadata['tabs'] = value
 
-    def add_tab_reference(self, tab_module):
-        '''
-        Since in CMS we can add static tabs via data, the current data model expects that
-        the list of tabs be encapsulated in the course.
-        VS[compat] we can rework this to avoid pointers to static tab modules once we fully deprecate filesystem support
-        '''
-        existing_tabs = self.tabs or []
-        existing_tabs.append({'type':'static_tab', 'name' : tab_module.metadata.get('display_name'), 'url_slug' : tab_module.location.name})
-        self.tabs = existing_tabs
-
-
-    def update_tab_reference(self, tab_module):
-        '''
-        Since in CMS we can add static tabs via data, the current data model expects that
-        the list of tabs be encapsulated in the course.
-        VS[compat] we can rework this to avoid pointers to static tab modules once we fully deprecate filesystem support
-        '''
-        existing_tabs = self.tabs
-        for tab in existing_tabs:
-            if tab.get('url_slug') == tab_module.location.name:
-                tab['name'] = tab_module.metadata.get('display_name')
-        self.tabs = existing_tabs
-
     @property
     def show_calculator(self):
         return self.metadata.get("show_calculator", None) == "Yes"

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

@@ -276,10 +276,49 @@ class MongoModuleStore(ModuleStoreBase):
             source_item = self.collection.find_one(location_to_query(source))
             source_item['_id'] = Location(location).dict()
             self.collection.insert(source_item)
-            return self._load_items([source_item])[0]
+            item = self._load_items([source_item])[0]
+
+            # VS[compat] cdodge: This is a hack because static_tabs also have references from the course module, so
+            # if we add one then we need to also add it to the policy information (i.e. metadata)
+            # we should remove this once we can break this reference from the course to static tabs
+            if location.category == 'static_tab':
+                course = self.get_course_for_item(item.location)
+                existing_tabs = course.tabs or []
+                existing_tabs.append({'type':'static_tab', 'name' : item.metadata.get('display_name'), 'url_slug' : item.location.name})
+                course.tabs = existing_tabs
+                self.update_metadata(course.location, course.metadata)
+
+            return item
         except pymongo.errors.DuplicateKeyError:
             raise DuplicateItemError(location)
 
+
+    def get_course_for_item(self, location):
+        '''
+        VS[compat]
+        cdodge: for a given Xmodule, return the course that it belongs to
+        NOTE: This makes a lot of assumptions about the format of the course location
+        Also we have to assert that this module maps to only one course item - it'll throw an
+        assert if not
+        This is only used to support static_tabs as we need to be course module aware
+        '''
+
+        # @hack! We need to find the course location however, we don't
+        # know the 'name' parameter in this context, so we have
+        # to assume there's only one item in this query even though we are not specifying a name
+        course_search_location = ['i4x', location.org, location.course, 'course', None]
+        courses = self.get_items(course_search_location)
+
+        # make sure we found exactly one match on this above course search
+        found_cnt = len(courses)
+        if found_cnt == 0:
+            raise BaseException('Could not find course at {0}'.format(course_search_location))
+
+        if found_cnt > 1:
+            raise BaseException('Found more than one course at {0}. There should only be one!!! Dump = {1}'.format(course_search_location, courses))
+
+        return courses[0]
+
     def _update_single_item(self, location, update):
         """
         Set update on the specified item, and raises ItemNotFoundError
@@ -327,6 +366,19 @@ class MongoModuleStore(ModuleStoreBase):
         location: Something that can be passed to Location
         metadata: A nested dictionary of module metadata
         """
+        # VS[compat] cdodge: This is a hack because static_tabs also have references from the course module, so
+        # if we add one then we need to also add it to the policy information (i.e. metadata)
+        # we should remove this once we can break this reference from the course to static tabs
+        loc = Location(location)
+        if loc.category == 'static_tab':
+            course = self.get_course_for_item(loc)
+            existing_tabs = course.tabs or []
+            for tab in existing_tabs:
+                if tab.get('url_slug') == loc.name:
+                    tab['name'] = metadata.get('display_name')
+                    break
+            course.tabs = existing_tabs
+            self.update_metadata(course.location, course.metadata)
 
         self._update_single_item(location, {'metadata': metadata})
 
@@ -336,6 +388,16 @@ class MongoModuleStore(ModuleStoreBase):
 
         location: Something that can be passed to Location
         """
+        # VS[compat] cdodge: This is a hack because static_tabs also have references from the course module, so
+        # if we add one then we need to also add it to the policy information (i.e. metadata)
+        # we should remove this once we can break this reference from the course to static tabs
+        if location.category == 'static_tab':
+            item = self.get_item(location)
+            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]
+            self.update_metadata(course.location, course.metadata)
+
         self.collection.remove({'_id': Location(location).dict()})
 
     def get_parent_locations(self, location):

common/lib/xmodule/xmodule/templates/about/empty.yaml

 ---
 metadata:
     display_name: Empty
-data: "This is where you can add additional information about your course."
+data: "<p>This is where you can add additional information about your course.</p>"
 children: []
\ No newline at end of file

common/lib/xmodule/xmodule/templates/courseinfo/empty.yaml

 ---
 metadata:
     display_name: Empty
-data: "This is where you can add additional information about your course."
+data: "<p>This is where you can add additional information about your course.</p>"
 children: []
\ No newline at end of file

common/lib/xmodule/xmodule/templates/statictab/empty.yaml

 ---
 metadata:
     display_name: Empty
-data: "This is where you can add additional pages to your courseware. Click the 'edit' button to begin editing."
+data: "<p>This is where you can add additional pages to your courseware. Click the 'edit' button to begin editing.</p>"
 children: []
\ No newline at end of file