Restful api for course advanced settings

STUD-948

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -1660,15 +1660,7 @@ class ContentStoreTest(ModuleStoreTestCase):
         test_get_html('tabs')
         test_get_html('settings/details')
         test_get_html('settings/grading')
-
-        # advanced settings
-        resp = self.client.get_html(reverse('course_advanced_settings',
-                                            kwargs={'org': loc.org,
-                                                    'course': loc.course,
-                                                    'name': loc.name}))
-        self.assertEqual(resp.status_code, 200)
-        # TODO: uncomment when advanced settings not using old locations.
-        # _test_no_locations(self, resp)
+        test_get_html('settings/advanced')
 
         # textbook index
         resp = self.client.get_html(reverse('textbook_index',

cms/djangoapps/contentstore/tests/test_course_settings.py

@@ -9,10 +9,9 @@ import mock
 from django.utils.timezone import UTC
 from django.test.utils import override_settings
 
-from xmodule.modulestore import Location
 from models.settings.course_details import (CourseDetails, CourseSettingsEncoder)
 from models.settings.course_grading import CourseGradingModel
-from contentstore.utils import get_modulestore
+from contentstore.utils import get_modulestore, EXTRA_TAB_PANELS
 from xmodule.modulestore.tests.factories import CourseFactory
 
 
@@ -20,7 +19,8 @@ from models.settings.course_metadata import CourseMetadata
 from xmodule.fields import Date
 
 from .utils import CourseTestCase
-from xmodule.modulestore.django import loc_mapper
+from xmodule.modulestore.django import loc_mapper, modulestore
+from contentstore.views.component import ADVANCED_COMPONENT_POLICY_KEY
 
 
 class CourseDetailsTestCase(CourseTestCase):
@@ -418,15 +418,19 @@ class CourseMetadataEditingTest(CourseTestCase):
     """
     def setUp(self):
         CourseTestCase.setUp(self)
-        CourseFactory.create(org='edX', course='999', display_name='Robot Super Course')
-        self.fullcourse_location = Location(['i4x', 'edX', '999', 'course', 'Robot_Super_Course', None])
+        self.fullcourse = CourseFactory.create(org='edX', course='999', display_name='Robot Super Course')
+        self.course_setting_url = self.course_locator.url_reverse('settings/advanced')
+        self.fullcourse_setting_url = loc_mapper().translate_location(
+            self.fullcourse.location.course_id,
+            self.fullcourse.location, False, True
+        ).url_reverse('settings/advanced')
 
     def test_fetch_initial_fields(self):
-        test_model = CourseMetadata.fetch(self.course.location)
+        test_model = CourseMetadata.fetch(self.course)
         self.assertIn('display_name', test_model, 'Missing editable metadata field')
         self.assertEqual(test_model['display_name'], 'Robot Super Course', "not expected value")
 
-        test_model = CourseMetadata.fetch(self.fullcourse_location)
+        test_model = CourseMetadata.fetch(self.fullcourse)
         self.assertNotIn('graceperiod', test_model, 'blacklisted field leaked in')
         self.assertIn('display_name', test_model, 'full missing editable metadata field')
         self.assertEqual(test_model['display_name'], 'Robot Super Course', "not expected value")
@@ -435,17 +439,18 @@ class CourseMetadataEditingTest(CourseTestCase):
         self.assertIn('xqa_key', test_model, 'xqa_key field ')
 
     def test_update_from_json(self):
-        test_model = CourseMetadata.update_from_json(self.course.location, {
+        test_model = CourseMetadata.update_from_json(self.course, {
             "advertised_start": "start A",
             "testcenter_info": {"c": "test"},
             "days_early_for_beta": 2
         })
         self.update_check(test_model)
         # try fresh fetch to ensure persistence
-        test_model = CourseMetadata.fetch(self.course.location)
+        fresh = modulestore().get_item(self.course_location)
+        test_model = CourseMetadata.fetch(fresh)
         self.update_check(test_model)
         # now change some of the existing metadata
-        test_model = CourseMetadata.update_from_json(self.course.location, {
+        test_model = CourseMetadata.update_from_json(fresh, {
             "advertised_start": "start B",
             "display_name": "jolly roger"}
         )
@@ -465,7 +470,11 @@ class CourseMetadataEditingTest(CourseTestCase):
         self.assertEqual(test_model['days_early_for_beta'], 2, "days_early_for_beta not expected value")
 
     def test_delete_key(self):
-        test_model = CourseMetadata.delete_key(self.fullcourse_location, {'deleteKeys': ['doesnt_exist', 'showanswer', 'xqa_key']})
+        test_model = CourseMetadata.update_from_json(
+            self.fullcourse, {
+                "unsetKeys": ['showanswer', 'xqa_key']
+            }
+        )
         # ensure no harm
         self.assertNotIn('graceperiod', test_model, 'blacklisted field leaked in')
         self.assertIn('display_name', test_model, 'full missing editable metadata field')
@@ -475,6 +484,65 @@ class CourseMetadataEditingTest(CourseTestCase):
         self.assertEqual('finished', test_model['showanswer'], 'showanswer field still in')
         self.assertEqual(None, test_model['xqa_key'], 'xqa_key field still in')
 
+    def test_http_fetch_initial_fields(self):
+        response = self.client.get_json(self.course_setting_url)
+        test_model = json.loads(response.content)
+        self.assertIn('display_name', test_model, 'Missing editable metadata field')
+        self.assertEqual(test_model['display_name'], 'Robot Super Course', "not expected value")
+
+        response = self.client.get_json(self.fullcourse_setting_url)
+        test_model = json.loads(response.content)
+        self.assertNotIn('graceperiod', test_model, 'blacklisted field leaked in')
+        self.assertIn('display_name', test_model, 'full missing editable metadata field')
+        self.assertEqual(test_model['display_name'], 'Robot Super Course', "not expected value")
+        self.assertIn('rerandomize', test_model, 'Missing rerandomize metadata field')
+        self.assertIn('showanswer', test_model, 'showanswer field ')
+        self.assertIn('xqa_key', test_model, 'xqa_key field ')
+
+    def test_http_update_from_json(self):
+        response = self.client.ajax_post(self.course_setting_url, {
+            "advertised_start": "start A",
+            "testcenter_info": {"c": "test"},
+            "days_early_for_beta": 2,
+            "unsetKeys": ['showanswer', 'xqa_key'],
+        })
+        test_model = json.loads(response.content)
+        self.update_check(test_model)
+        self.assertEqual('finished', test_model['showanswer'], 'showanswer field still in')
+        self.assertEqual(None, test_model['xqa_key'], 'xqa_key field still in')
+
+        response = self.client.get_json(self.course_setting_url)
+        test_model = json.loads(response.content)
+        self.update_check(test_model)
+        # now change some of the existing metadata
+        response = self.client.ajax_post(self.course_setting_url, {
+            "advertised_start": "start B",
+            "display_name": "jolly roger"
+        })
+        test_model = json.loads(response.content)
+        self.assertIn('display_name', test_model, 'Missing editable metadata field')
+        self.assertEqual(test_model['display_name'], 'jolly roger', "not expected value")
+        self.assertIn('advertised_start', test_model, 'Missing revised advertised_start metadata field')
+        self.assertEqual(test_model['advertised_start'], 'start B', "advertised_start not expected value")
+
+    def test_advanced_components_munge_tabs(self):
+        """
+        Test that adding and removing specific advanced components adds and removes tabs.
+        """
+        self.assertNotIn(EXTRA_TAB_PANELS.get("open_ended"), self.course.tabs)
+        self.assertNotIn(EXTRA_TAB_PANELS.get("notes"), self.course.tabs)
+        self.client.ajax_post(self.course_setting_url, {
+            ADVANCED_COMPONENT_POLICY_KEY: ["combinedopenended"]
+        })
+        course = modulestore().get_item(self.course_location)
+        self.assertIn(EXTRA_TAB_PANELS.get("open_ended"), course.tabs)
+        self.assertNotIn(EXTRA_TAB_PANELS.get("notes"), course.tabs)
+        self.client.ajax_post(self.course_setting_url, {
+            ADVANCED_COMPONENT_POLICY_KEY: []
+        })
+        course = modulestore().get_item(self.course_location)
+        self.assertNotIn(EXTRA_TAB_PANELS.get("open_ended"), course.tabs)
+
 
 class CourseGraderUpdatesTest(CourseTestCase):
     """

cms/djangoapps/models/settings/course_details.py

@@ -32,11 +32,11 @@ class CourseDetails(object):
         self.course_image_asset_path = ""  # URL of the course image
 
     @classmethod
-    def fetch(cls, course_location):
+    def fetch(cls, course_locator):
         """
         Fetch the course details for the given course from persistence and return a CourseDetails model.
         """
-        course_old_location = loc_mapper().translate_locator_to_location(course_location)
+        course_old_location = loc_mapper().translate_locator_to_location(course_locator)
         descriptor = get_modulestore(course_old_location).get_item(course_old_location)
         course = cls(course_old_location.org, course_old_location.course, course_old_location.name)
 
@@ -75,11 +75,11 @@ class CourseDetails(object):
         return course
 
     @classmethod
-    def update_from_json(cls, course_location, jsondict):
+    def update_from_json(cls, course_locator, jsondict):
         """
         Decode the json into CourseDetails and save any changed attrs to the db
         """
-        course_old_location = loc_mapper().translate_locator_to_location(course_location)
+        course_old_location = loc_mapper().translate_locator_to_location(course_locator)
         descriptor = get_modulestore(course_old_location).get_item(course_old_location)
 
         dirty = False
@@ -153,7 +153,7 @@ class CourseDetails(object):
 
         # Could just return jsondict w/o doing any db reads, but I put the reads in as a means to confirm
         # it persisted correctly
-        return CourseDetails.fetch(course_location)
+        return CourseDetails.fetch(course_locator)
 
     @staticmethod
     def parse_video_tag(raw_video):

cms/djangoapps/models/settings/course_grading.py

@@ -18,11 +18,11 @@ class CourseGradingModel(object):
         self.grace_period = CourseGradingModel.convert_set_grace_period(course_descriptor)
 
     @classmethod
-    def fetch(cls, course_location):
+    def fetch(cls, course_locator):
         """
         Fetch the course grading policy for the given course from persistence and return a CourseGradingModel.
         """
-        course_old_location = loc_mapper().translate_locator_to_location(course_location)
+        course_old_location = loc_mapper().translate_locator_to_location(course_locator)
         descriptor = get_modulestore(course_old_location).get_item(course_old_location)
 
         model = cls(descriptor)
@@ -52,12 +52,12 @@ class CourseGradingModel(object):
                     }
 
     @staticmethod
-    def update_from_json(course_location, jsondict):
+    def update_from_json(course_locator, jsondict):
         """
         Decode the json into CourseGradingModel and save any changes. Returns the modified model.
         Probably not the usual path for updates as it's too coarse grained.
         """
-        course_old_location = loc_mapper().translate_locator_to_location(course_location)
+        course_old_location = loc_mapper().translate_locator_to_location(course_locator)
         descriptor = get_modulestore(course_old_location).get_item(course_old_location)
 
         graders_parsed = [CourseGradingModel.parse_grader(jsonele) for jsonele in jsondict['graders']]
@@ -69,9 +69,9 @@ class CourseGradingModel(object):
             course_old_location, descriptor.get_explicitly_set_fields_by_scope(Scope.content)
         )
 
-        CourseGradingModel.update_grace_period_from_json(course_location, jsondict['grace_period'])
+        CourseGradingModel.update_grace_period_from_json(course_locator, jsondict['grace_period'])
 
-        return CourseGradingModel.fetch(course_location)
+        return CourseGradingModel.fetch(course_locator)
 
     @staticmethod
     def update_grader_from_json(course_location, grader):

cms/djangoapps/models/settings/course_metadata.py

-from xmodule.modulestore import Location
+from xblock.fields import Scope
+
 from contentstore.utils import get_modulestore
 from xmodule.modulestore.inheritance import own_metadata
-from xblock.fields import Scope
 from cms.xmodule_namespace import CmsBlockMixin
 
 
@@ -20,21 +20,18 @@ class CourseMetadata(object):
                      'tabs',
                      'graceperiod',
                      'checklists',
-                     'show_timezone'
+                     'show_timezone',
+                     'format',
+                     'graded',
     ]
 
     @classmethod
-    def fetch(cls, course_location):
+    def fetch(cls, descriptor):
         """
         Fetch the key:value editable course details for the given course from
         persistence and return a CourseMetadata model.
         """
-        if not isinstance(course_location, Location):
-            course_location = Location(course_location)
-
-        course = {}
-
-        descriptor = get_modulestore(course_location).get_item(course_location)
+        result = {}
 
         for field in descriptor.fields.values():
             if field.name in CmsBlockMixin.fields:
@@ -46,19 +43,17 @@ class CourseMetadata(object):
             if field.name in cls.FILTERED_LIST:
                 continue
 
-            course[field.name] = field.read_json(descriptor)
+            result[field.name] = field.read_json(descriptor)
 
-        return course
+        return result
 
     @classmethod
-    def update_from_json(cls, course_location, jsondict, filter_tabs=True):
+    def update_from_json(cls, descriptor, jsondict, filter_tabs=True):
         """
         Decode the json into CourseMetadata and save any changed attrs to the db.
 
         Ensures none of the fields are in the blacklist.
         """
-        descriptor = get_modulestore(course_location).get_item(course_location)
-
         dirty = False
 
         # Copy the filtered list to avoid permanently changing the class attribute.
@@ -72,39 +67,17 @@ class CourseMetadata(object):
             if key in filtered_list:
                 continue
 
+            if key == "unsetKeys":
+                dirty = True
+                for unset in val:
+                    descriptor.fields[unset].delete_from(descriptor)
+
             if hasattr(descriptor, key) and getattr(descriptor, key) != val:
                 dirty = True
                 value = descriptor.fields[key].from_json(val)
                 setattr(descriptor, key, value)
 
         if dirty:
-            # Save the data that we've just changed to the underlying
-            # MongoKeyValueStore before we update the mongo datastore.
-            descriptor.save()
-            get_modulestore(course_location).update_metadata(course_location,
-                                                             own_metadata(descriptor))
-
-        # Could just generate and return a course obj w/o doing any db reads,
-        # but I put the reads in as a means to confirm it persisted correctly
-        return cls.fetch(course_location)
-
-    @classmethod
-    def delete_key(cls, course_location, payload):
-        '''
-        Remove the given metadata key(s) from the course. payload can be a
-        single key or [key..]
-        '''
-        descriptor = get_modulestore(course_location).get_item(course_location)
-
-        for key in payload['deleteKeys']:
-            if hasattr(descriptor, key):
-                delattr(descriptor, key)
-
-        # Save the data that we've just changed to the underlying
-        # MongoKeyValueStore before we update the mongo datastore.
-        descriptor.save()
-
-        get_modulestore(course_location).update_metadata(course_location,
-                                                         own_metadata(descriptor))
+            get_modulestore(descriptor.location).update_metadata(descriptor.location, own_metadata(descriptor))
 
-        return cls.fetch(course_location)
+        return cls.fetch(descriptor)

cms/templates/settings.html

@@ -6,7 +6,6 @@
 <%!
   from django.utils.translation import ugettext as _
   from xmodule.modulestore.django import loc_mapper
-  from django.core.urlresolvers import reverse
 %>
 
 <%block name="jsextra">
@@ -293,14 +292,14 @@ require(["domReady!", "jquery", "js/models/settings/course_details", "js/views/s
           <%
             course_team_url = course_locator.url_reverse('course_team/', '')
             grading_config_url = course_locator.url_reverse('settings/grading/')
-            ctx_loc = context_course.location
+            advanced_config_url = course_locator.url_reverse('settings/advanced/')
           %>
         <h3 class="title-3">${_("Other Course Settings")}</h3>
         <nav class="nav-related">
           <ul>
             <li class="nav-item"><a href="${grading_config_url}">${_("Grading")}</a></li>
             <li class="nav-item"><a href="${course_team_url}">${_("Course Team")}</a></li>
-            <li class="nav-item"><a href="${reverse('course_advanced_settings', kwargs={'org' : ctx_loc.org, 'course' : ctx_loc.course, 'name': ctx_loc.name})}">${_("Advanced Settings")}</a></li>
+            <li class="nav-item"><a href="${advanced_config_url}">${_("Advanced Settings")}</a></li>
           </ul>
         </nav>
      % endif

cms/templates/settings_advanced.html

 <%inherit file="base.html" />
 <%namespace name='static' file='static_content.html'/>
 <%!
-  from django.core.urlresolvers import reverse
   from django.utils.translation import ugettext as _
   from contentstore import utils
   from xmodule.modulestore.django import loc_mapper
-  from django.core.urlresolvers import reverse
 %>
 <%block name="title">${_("Advanced Settings")}</%block>
 <%block name="bodyclass">is-signedin course advanced view-settings</%block>
@@ -28,7 +26,7 @@ require(["domReady!", "jquery", "js/models/settings/advanced", "js/views/setting
 
   // proactively populate advanced b/c it has the filtered list and doesn't really follow the model pattern
   var advancedModel = new AdvancedSettingsModel(${advanced_dict | n}, {parse: true});
-  advancedModel.url = "${reverse('course_advanced_settings_updates', kwargs=dict(org=context_course.location.org, course=context_course.location.course, name=context_course.location.name))}";
+  advancedModel.url = "${advanced_settings_url}";
 
   var editor = new AdvancedSettingsView({
     el: $('.settings-advanced'),
@@ -91,13 +89,15 @@ require(["domReady!", "jquery", "js/models/settings/advanced", "js/views/setting
         <%
           ctx_loc = context_course.location
           location = loc_mapper().translate_location(ctx_loc.course_id, ctx_loc, False, True)
+          details_url = location.url_reverse('settings/details/')
+          grading_url = location.url_reverse('settings/grading/')
           course_team_url = location.url_reverse('course_team/', '')
         %>
       <h3 class="title-3">${_("Other Course Settings")}</h3>
         <nav class="nav-related">
           <ul>
-            <li class="nav-item"><a href="${course_locator.url_reverse('settings/details/')}">${_("Details &amp; Schedule")}</a></li>
-            <li class="nav-item"><a href="${course_locator.url_reverse('settings/grading/')}">${_("Grading")}</a></li>
+            <li class="nav-item"><a href="${details_url}">${_("Details &amp; Schedule")}</a></li>
+            <li class="nav-item"><a href="${grading_url}">${_("Grading")}</a></li>
             <li class="nav-item"><a href="${course_team_url}">${_("Course Team")}</a></li>
           </ul>
         </nav>

cms/templates/settings_graders.html

@@ -7,7 +7,6 @@
   from contentstore import utils
   from django.utils.translation import ugettext as _
   from xmodule.modulestore.django import loc_mapper
-  from django.core.urlresolvers import reverse
 %>
 
 <%block name="header_extras">
@@ -139,15 +138,16 @@ require(["domReady!", "jquery", "js/views/settings/grading", "js/models/settings
       <div class="bit">
       % if context_course:
         <%
-          ctx_loc = context_course.location
           course_team_url = course_locator.url_reverse('course_team/')
+          advanced_settings_url = course_locator.url_reverse('settings/advanced/')
+          detailed_settings_url = course_locator.url_reverse('settings/details/')
         %>
         <h3 class="title-3">${_("Other Course Settings")}</h3>
         <nav class="nav-related">
           <ul>
-            <li class="nav-item"><a href="${course_locator.url_reverse('settings/details/')}">${_("Details &amp; Schedule")}</a></li>
+            <li class="nav-item"><a href="${detailed_settings_url}">${_("Details &amp; Schedule")}</a></li>
             <li class="nav-item"><a href="${course_team_url}">${_("Course Team")}</a></li>
-            <li class="nav-item"><a href="${reverse('course_advanced_settings', kwargs={'org' : ctx_loc.org, 'course' : ctx_loc.course, 'name': ctx_loc.name})}">${_("Advanced Settings")}</a></li>
+            <li class="nav-item"><a href="${advanced_settings_url}">${_("Advanced Settings")}</a></li>
           </ul>
         </nav>
       % endif

cms/templates/widgets/header.html

@@ -25,6 +25,7 @@
             export_url = location.url_reverse('export')
             settings_url = location.url_reverse('settings/details/')
             grading_url = location.url_reverse('settings/grading/')
+            advanced_settings_url = location.url_reverse('settings/advanced/')
             tabs_url = location.url_reverse('tabs')
       %>
       <h2 class="info-course">
@@ -80,7 +81,7 @@
                     <a href="${course_team_url}">${_("Course Team")}</a>
                   </li>
                   <li class="nav-item nav-course-settings-advanced">
-                    <a href="${reverse('course_advanced_settings', kwargs={'org' : ctx_loc.org, 'course' : ctx_loc.course, 'name': ctx_loc.name})}">${_("Advanced Settings")}</a>
+                    <a href="${advanced_settings_url}">${_("Advanced Settings")}</a>
                   </li>
                 </ul>
               </div>

cms/urls.py

@@ -23,13 +23,6 @@ urlpatterns = patterns('',  # nopep8
     url(r'^preview/xblock/(?P<usage_id>.*?)/handler/(?P<handler>[^/]*)(?:/(?P<suffix>[^/]*))?$',
         'contentstore.views.preview_handler', name='preview_handler'),
 
-    # This is the URL to initially render the course advanced settings.
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/settings-advanced/(?P<name>[^/]+)$',
-        'contentstore.views.course_config_advanced_page', name='course_advanced_settings'),
-    # This is the URL used by BackBone for updating and re-fetching the model.
-    url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/settings-advanced/(?P<name>[^/]+)/update.*$',
-        'contentstore.views.course_advanced_updates', name='course_advanced_settings_updates'),
-
     url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/textbooks/(?P<name>[^/]+)$',
         'contentstore.views.textbook_index', name='textbook_index'),
     url(r'^(?P<org>[^/]+)/(?P<course>[^/]+)/textbooks/(?P<name>[^/]+)/new$',
@@ -95,6 +88,7 @@ urlpatterns += patterns(
     url(r'(?ix)^tabs/{}$'.format(parsers.URL_RE_SOURCE), 'tabs_handler'),
     url(r'(?ix)^settings/details/{}$'.format(parsers.URL_RE_SOURCE), 'settings_handler'),
     url(r'(?ix)^settings/grading/{}(/)?(?P<grader_index>\d+)?$'.format(parsers.URL_RE_SOURCE), 'grading_handler'),
+    url(r'(?ix)^settings/advanced/{}$'.format(parsers.URL_RE_SOURCE), 'advanced_settings_handler'),
 )
 
 js_info_dict = {

common/djangoapps/util/json_request.py

 from functools import wraps
-import copy
 import json
 from django.core.serializers import serialize
 from django.core.serializers.json import DjangoJSONEncoder