Cannot set String field to a dict anymore!

cms/djangoapps/contentstore/features/advanced-settings.feature

@@ -28,11 +28,16 @@ Feature: Advanced (manual) course policy
 
   Scenario: Test how multi-line input appears
     Given I am on the Advanced Course Settings page in Studio
-    When I create a JSON object as a value
+    When I create a JSON object as a value for "discussion_topics"
     Then it is displayed as formatted
     And I reload the page
     Then it is displayed as formatted
 
+  Scenario: Test error if value supplied is of the wrong type
+    Given I am on the Advanced Course Settings page in Studio
+    When I create a JSON object as a value for "display_name"
+    Then I get an error on save
+
   Scenario: Test automatic quoting of non-JSON values
     Given I am on the Advanced Course Settings page in Studio
     When I create a non-JSON value not in quotes

cms/djangoapps/contentstore/features/advanced-settings.py

@@ -2,8 +2,7 @@
 #pylint: disable=W0621
 
 from lettuce import world, step
-from common import *
-from nose.tools import assert_false, assert_equal
+from nose.tools import assert_false, assert_equal, assert_regexp_matches
 
 """
 http://selenium.googlecode.com/svn/trunk/docs/api/py/webdriver/selenium.webdriver.common.keys.html
@@ -52,9 +51,9 @@ def edit_the_value_of_a_policy_key_and_save(step):
     change_display_name_value(step, '"foo"')
 
 
-@step('I create a JSON object as a value$')
-def create_JSON_object(step):
-    change_display_name_value(step, '{"key": "value", "key_2": "value_2"}')
+@step('I create a JSON object as a value for "(.*)"$')
+def create_JSON_object(step, key):
+    change_value(step, key, '{"key": "value", "key_2": "value_2"}')
 
 
 @step('I create a non-JSON value not in quotes$')
@@ -82,7 +81,12 @@ def they_are_alphabetized(step):
 
 @step('it is displayed as formatted$')
 def it_is_formatted(step):
-    assert_policy_entries([DISPLAY_NAME_KEY], ['{\n    "key": "value",\n    "key_2": "value_2"\n}'])
+    assert_policy_entries(['discussion_topics'], ['{\n    "key": "value",\n    "key_2": "value_2"\n}'])
+
+
+@step('I get an error on save$')
+def error_on_save(step):
+    assert_regexp_matches(world.css_text('#notification-error-description'), 'Incorrect setting format')
 
 
 @step('it is displayed as a string')
@@ -124,11 +128,16 @@ def get_display_name_value():
 
 
 def change_display_name_value(step, new_value):
+    change_value(step, DISPLAY_NAME_KEY, new_value)
 
-    world.css_find(".CodeMirror")[get_index_of(DISPLAY_NAME_KEY)].click()
+
+def change_value(step, key, new_value):
+    index = get_index_of(key)
+    world.css_find(".CodeMirror")[index].click()
     g = world.css_find("div.CodeMirror.CodeMirror-focused > div > textarea")
-    display_name = get_display_name_value()
-    for count in range(len(display_name)):
+    current_value = world.css_find(VALUE_CSS)[index].value
+    g._element.send_keys(Keys.CONTROL + Keys.END)
+    for count in range(len(current_value)):
         g._element.send_keys(Keys.END, Keys.BACK_SPACE)
         # Must delete "" before typing the JSON value
     g._element.send_keys(Keys.END, Keys.BACK_SPACE, Keys.BACK_SPACE, new_value)

cms/djangoapps/contentstore/features/problem-editor.py

@@ -41,7 +41,9 @@ def i_see_five_settings_with_values(step):
 
 @step('I can modify the display name')
 def i_can_modify_the_display_name(step):
-    world.get_setting_entry(DISPLAY_NAME).find_by_css('.setting-input')[0].fill('modified')
+    # Verifying that the display name can be a string containing a floating point value
+    # (to confirm that we don't throw an error because it is of the wrong type).
+    world.get_setting_entry(DISPLAY_NAME).find_by_css('.setting-input')[0].fill('3.4')
     verify_modified_display_name()
 
 
@@ -172,7 +174,7 @@ def verify_modified_randomization():
 
 
 def verify_modified_display_name():
-    world.verify_setting_entry(world.get_setting_entry(DISPLAY_NAME), DISPLAY_NAME, 'modified', True)
+    world.verify_setting_entry(world.get_setting_entry(DISPLAY_NAME), DISPLAY_NAME, '3.4', True)
 
 
 def verify_modified_display_name_with_special_chars():

cms/djangoapps/contentstore/views/course.py

@@ -401,8 +401,11 @@ def course_advanced_updates(request, org, course, name):
                         request_body.update({'tabs': new_tabs})
                         #Indicate that tabs should *not* be filtered out of the metadata
                         filter_tabs = False
-
-        response_json = json.dumps(CourseMetadata.update_from_json(location,
+        try:
+            response_json = json.dumps(CourseMetadata.update_from_json(location,
                                                                    request_body,
                                                                    filter_tabs=filter_tabs))
+        except (TypeError, ValueError), e:
+            return HttpResponseBadRequest("Incorrect setting format. " + str(e), content_type="text/plain")
+
         return HttpResponse(response_json, mimetype="application/json")

common/lib/xmodule/xmodule/tests/test_xml_module.py

@@ -188,6 +188,18 @@ def assertDeserializeEqual(field, expected, arg):
     assert_equals(expected, deserialize_field(field, arg))
 
 
+def assertDeserializeNonString(field):
+    """
+    Asserts input value is returned for None or something that is not a string.
+    """
+    assertDeserializeEqual(field, None, None)
+    assertDeserializeEqual(field, 3.14, 3.14)
+    assertDeserializeEqual(field, True, True)
+    assertDeserializeEqual(field, [10], [10])
+    assertDeserializeEqual(field, {}, {})
+    assertDeserializeEqual(field, [], [])
+
+
 class TestSerializeInteger(unittest.TestCase):
     """ Tests serialize/deserialize as related to Integer type. """
 
@@ -208,7 +220,7 @@ class TestSerializeInteger(unittest.TestCase):
 
     def test_deserialize_unsupported_types(self):
         assertDeserializeEqual(Integer(), '[3]', '[3]')
-        self.assertRaises(TypeError, deserialize_field, None)
+        assertDeserializeNonString(Integer())
 
 
 class FloatTest(unittest.TestCase):
@@ -235,7 +247,7 @@ class FloatTest(unittest.TestCase):
 
     def test_deserialize_unsupported_types(self):
         assertDeserializeEqual(Float(), '[3]', '[3]')
-        self.assertRaises(TypeError, deserialize_field, None)
+        assertDeserializeNonString(Float())
 
 
 class BooleanTest(unittest.TestCase):
@@ -264,8 +276,7 @@ class BooleanTest(unittest.TestCase):
         assertDeserializeEqual(Boolean(), 'fAlse', '"fAlse"')
         assertDeserializeEqual(Boolean(), "TruE", '"TruE"')
 
-    def test_deserialize_unsupported_types(self):
-        self.assertRaises(TypeError, deserialize_field, None)
+        assertDeserializeNonString(Boolean())
 
 
 class StringTest(unittest.TestCase):
@@ -286,7 +297,7 @@ class StringTest(unittest.TestCase):
         assertDeserializeEqual(String(), 'false', 'false')
         assertDeserializeEqual(String(), '2', '2')
         assertDeserializeEqual(String(), '[3]', '[3]')
-        self.assertRaises(TypeError, deserialize_field, None)
+        assertDeserializeNonString(String())
 
 
 class AnyTest(unittest.TestCase):
@@ -307,9 +318,7 @@ class AnyTest(unittest.TestCase):
         assertDeserializeEqual(Any(), '[', '[')
         assertDeserializeEqual(Any(), False, 'false')
         assertDeserializeEqual(Any(), 3.4, '3.4')
-
-    def test_deserialize_unsupported_types(self):
-        self.assertRaises(TypeError, deserialize_field, None)
+        assertDeserializeNonString(Any())
 
 
 class ListTest(unittest.TestCase):
@@ -330,7 +339,7 @@ class ListTest(unittest.TestCase):
         assertDeserializeEqual(List(), '3.4', '3.4')
         assertDeserializeEqual(List(), 'false', 'false')
         assertDeserializeEqual(List(), '2', '2')
-        self.assertRaises(TypeError, deserialize_field, None)
+        assertDeserializeNonString(List())
 
 
 class DateTest(unittest.TestCase):
@@ -342,9 +351,11 @@ class DateTest(unittest.TestCase):
     def test_deserialize(self):
         assertDeserializeEqual(Date(), '2012-12-31T23:59:59Z', "2012-12-31T23:59:59Z")
         assertDeserializeEqual(Date(), '2012-12-31T23:59:59Z', '"2012-12-31T23:59:59Z"')
+        assertDeserializeNonString(Date())
 
 
 class TimedeltaTest(unittest.TestCase):
+    """ Tests serialize/deserialize as related to Timedelta type. """
 
     def test_serialize(self):
         assertSerializeEqual('"1 day 12 hours 59 minutes 59 seconds"',
@@ -355,3 +366,4 @@ class TimedeltaTest(unittest.TestCase):
             '1 day 12 hours 59 minutes 59 seconds')
         assertDeserializeEqual(Timedelta(), '1 day 12 hours 59 minutes 59 seconds',
             '"1 day 12 hours 59 minutes 59 seconds"')
+        assertDeserializeNonString(Timedelta())

common/lib/xmodule/xmodule/xml_module.py

@@ -95,23 +95,28 @@ def deserialize_field(field, value):
     Note that this is not the same as the value returned by from_json, as model types typically store
     their value internally as JSON. By default, this method will return the result of calling json.loads
     on the supplied value, unless json.loads throws a TypeError, or the type of the value returned by json.loads
-    is not supported for this class (see 'is_type_supported'). In either of those cases, this method returns
+    is not supported for this class (from_json throws an Error). In either of those cases, this method returns
     the input value.
     """
     try:
         deserialized = json.loads(value)
-
         if deserialized is None:
             return deserialized
         try:
             field.from_json(deserialized)
             return deserialized
         except (ValueError, TypeError):
-            # Support older serialized forms by simply returning the String representation
+            # Support older serialized version, which was just a string, not result of json.dumps.
+            # If the deserialized version cannot be converted to the type (via from_json),
+            # just return the original value. For example, if a string value of '3.4' was
+            # stored for a String field (before we started storing the result of json.dumps),
+            # then it would be deserialized as 3.4, but 3.4 is not supported for a String
+            # field. Therefore field.from_json(3.4) will throw an Error, and we should
+            # actually return the original value of '3.4'.
             return value
 
     except (ValueError, TypeError):
-        # Support older serialized version, which was just the String (not the result of json.dumps).
+        # Support older serialized version.
         return value