From 082216395c57d3f6c89e97178c4e5fa1ffe44fa8 Mon Sep 17 00:00:00 2001
From: Will Daly <will@edx.org>
Date: Mon, 4 Mar 2013 12:38:11 -0500
Subject: [PATCH] Added unit test for conversion of GET parameters for capa
 module. Added error checking for possible error cases

---
 common/lib/xmodule/xmodule/capa_module.py     | 35 ++++++++++---
 .../xmodule/xmodule/tests/test_capa_module.py | 50 +++++++++++++++++++
 2 files changed, 77 insertions(+), 8 deletions(-)

diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py
index ade5cc6ef6..05a12f1761 100644
--- a/common/lib/xmodule/xmodule/capa_module.py
+++ b/common/lib/xmodule/xmodule/capa_module.py
@@ -534,15 +534,34 @@ class CapaModule(XModule):
             # e.g. input_resistor_1 ==> resistor_1
             _, _, name = key.partition('_')
 
-            # This allows for answers which require more than one value for
-            # the same form input (e.g. checkbox inputs). The convention is that
-            # if the name ends with '[]' (which looks like an array), then the
-            # answer will be an array.
-            if not name.endswith('[]'):
-                answers[name] = get[key]
+            # If key has no underscores, then partition
+            # will return (key, '', '')
+            # We detect this and raise an error
+            if name is '':
+                raise ValueError("%s must contain at least one underscore" % str(key))
+
             else:
-                name = name[:-2]
-                answers[name] = get.getlist(key)
+                # This allows for answers which require more than one value for
+                # the same form input (e.g. checkbox inputs). The convention is that
+                # if the name ends with '[]' (which looks like an array), then the
+                # answer will be an array.
+                is_list_key = name.endswith('[]')
+                name = name[:-2] if is_list_key else name
+
+                if is_list_key:
+                    if type(get[key]) is list:
+                        val = get[key]
+                    else:
+                        val = [get[key]]
+                else:
+                    val = get[key]
+
+                # If the name already exists, then we don't want
+                # to override it.  Raise an error instead
+                if name in answers:
+                    raise ValueError("Key %s already exists in answers dict" % str(name))
+                else:
+                    answers[name] = val
 
         return answers
 
diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py
index e84267c1e7..7b565ff2da 100644
--- a/common/lib/xmodule/xmodule/tests/test_capa_module.py
+++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py
@@ -282,3 +282,53 @@ class CapaModuleTest(unittest.TestCase):
                                             due=self.yesterday_str,
                                             graceperiod=self.two_day_delta_str)
         self.assertTrue(still_in_grace.answer_available())
+
+
+    def test_parse_get_params(self):
+
+        # Valid GET param dict
+        valid_get_dict = {'input_1': 'test',
+                        'input_1_2': 'test',
+                        'input_1_2_3': 'test',
+                        'input_[]_3': 'test',
+                        'input_4': None,
+                        'input_5': [],
+                        'input_6': 5}
+        
+        result = CapaModule.make_dict_of_responses(valid_get_dict)
+
+        # Expect that we get a dict with "input" stripped from key names
+        # and that we get the same values back
+        for key in result.keys():
+            original_key = "input_" + key
+            self.assertTrue(original_key in valid_get_dict,
+                            "Output dict should have key %s" % original_key)
+            self.assertEqual(valid_get_dict[original_key], result[key])
+
+
+        # Valid GET param dict with list keys
+        valid_get_dict = {'input_2[]': ['test1', 'test2']}
+        result = CapaModule.make_dict_of_responses(valid_get_dict)
+        self.assertTrue('2' in result)
+        self.assertEqual(valid_get_dict['input_2[]'], result['2'])
+
+        # If we use [] at the end of a key name, we should always
+        # get a list, even if there's just one value
+        valid_get_dict = {'input_1[]': 'test'}
+        result = CapaModule.make_dict_of_responses(valid_get_dict)
+        self.assertEqual(result['1'], ['test'])
+
+
+        # If we have no underscores in the name, then the key is invalid
+        invalid_get_dict = {'input': 'test'}
+        with self.assertRaises(ValueError):
+            result = CapaModule.make_dict_of_responses(invalid_get_dict)
+
+
+        # Two equivalent names (one list, one non-list)
+        # One of the values would overwrite the other, so detect this
+        # and raise an exception
+        invalid_get_dict = {'input_1[]': 'test 1',
+                            'input_1': 'test 2' }
+        with self.assertRaises(ValueError):
+            result = CapaModule.make_dict_of_responses(invalid_get_dict)