From 6e92ddf3ddbd493310217a0f1179ce6c4fa55bca Mon Sep 17 00:00:00 2001 From: cahrens Date: Wed, 12 Jun 2013 17:07:41 -0400 Subject: [PATCH] Cannot set String field to a dict anymore! --- .../features/advanced-settings.feature | 7 ++++- .../features/advanced-settings.py | 27 +++++++++++------ .../contentstore/features/problem-editor.py | 6 ++-- cms/djangoapps/contentstore/views/course.py | 7 +++-- .../xmodule/xmodule/tests/test_xml_module.py | 30 +++++++++++++------ common/lib/xmodule/xmodule/xml_module.py | 13 +++++--- 6 files changed, 63 insertions(+), 27 deletions(-) diff --git a/cms/djangoapps/contentstore/features/advanced-settings.feature b/cms/djangoapps/contentstore/features/advanced-settings.feature index 558294e890..310c32d05a 100644 --- a/cms/djangoapps/contentstore/features/advanced-settings.feature +++ b/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 diff --git a/cms/djangoapps/contentstore/features/advanced-settings.py b/cms/djangoapps/contentstore/features/advanced-settings.py index eb00c06ba9..049103db27 100644 --- a/cms/djangoapps/contentstore/features/advanced-settings.py +++ b/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) diff --git a/cms/djangoapps/contentstore/features/problem-editor.py b/cms/djangoapps/contentstore/features/problem-editor.py index 5dfcf55046..7679128beb 100644 --- a/cms/djangoapps/contentstore/features/problem-editor.py +++ b/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(): diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 07f6b9669c..6f3f4ee155 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/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") diff --git a/common/lib/xmodule/xmodule/tests/test_xml_module.py b/common/lib/xmodule/xmodule/tests/test_xml_module.py index 46cd90b02c..0eb4a92dfa 100644 --- a/common/lib/xmodule/xmodule/tests/test_xml_module.py +++ b/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()) diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index b27cf97dfa..9b5de9d7e7 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/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