Repository: XLSForm/pyxform
Pull request #184 (2018-03-19 12:51:22)

Disallow Duplicate Column Names
Ensure that duplicate column names fail validation, even if they are in different cases.

Fix: #174


Patch:
diff --git a/pyxform/section.py b/pyxform/section.py
index 2be0b7a..9d26822 100644
--- a/pyxform/section.py
+++ b/pyxform/section.py
@@ -1,7 +1,7 @@
+from pyxform.errors import PyXFormError
 from pyxform.external_instance import ExternalInstance
 from pyxform.question import SurveyElement
 from pyxform.utils import node
-from pyxform.errors import PyXFormError
 
 
 class Section(SurveyElement):
@@ -16,11 +16,12 @@ class Section(SurveyElement):
     def _validate_uniqueness_of_element_names(self):
         element_slugs = []
         for element in self.children:
-            if element.name in element_slugs:
+            if any(element.name.lower() == s.lower() for s in element_slugs):
                 raise PyXFormError(
-                    "There are two survey elements named '%s' in the section"
-                    " named '%s'." % (element.name, self.name)
-                    )
+                    "There are more than one survey elements named '%s' "
+                    "(case-insensitive) in the section named '%s'." %
+                    (element.name.lower(), self.name)
+                )
             element_slugs.append(element.name)
 
     def xml_instance(self, **kwargs):
@@ -97,7 +98,7 @@ class RepeatingSection(Section):
             return node(
                 u"group", self.xml_label(), repeat_node,
                 ref=self.get_xpath()
-                )
+            )
         return node(u"group", repeat_node, ref=self.get_xpath(),
                     **self.control)
 
diff --git a/setup.cfg b/setup.cfg
index 3c6e79c..fbdb966 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -1,2 +1,7 @@
 [bdist_wheel]
 universal=1
+
+[isort]
+default_section = THIRDPARTY
+known_first_party = pyxform
+sections = FUTURE,STDLIB,THIRDPARTY,FIRSTPARTY,LOCALFOLDER
\ No newline at end of file


Test patch:
diff --git a/pyxform/tests_v1/test_fields.py b/pyxform/tests_v1/test_fields.py
new file mode 100644
index 0000000..58c4277
--- /dev/null
+++ b/pyxform/tests_v1/test_fields.py
@@ -0,0 +1,41 @@
+from pyxform.tests_v1.pyxform_test_case import PyxformTestCase
+
+
+class FieldsTests(PyxformTestCase):
+    """
+    Test XLSForm Fields
+    """
+
+    def test_duplicate_fields(self):
+        """
+        Ensure that duplicate field names are not allowed
+        """
+        self.assertPyxformXform(
+            name="duplicatefields",
+            md="""
+            | Survey |         |         |               |
+            |        | Type    | Name    | Label         |
+            |        | integer | age     | the age       |
+            |        | integer | age     | the age       |
+            """,
+            errored=True,
+            error__contains=[
+                "There are more than one survey elements named 'age'"],
+        )
+
+    def test_duplicate_fields_diff_cases(self):
+        """
+        Ensure that duplicate field names with different cases are not allowed
+        """
+        self.assertPyxformXform(
+            name="duplicatefieldsdiffcases",
+            md="""
+            | Survey |         |         |               |
+            |        | Type    | Name    | Label         |
+            |        | integer | age     | the age       |
+            |        | integer | Age     | the age       |
+            """,
+            errored=True,
+            error__contains=[
+                "There are more than one survey elements named 'age'"],
+        )
diff --git a/pyxform/tests_v1/test_sheet_columns.py b/pyxform/tests_v1/test_sheet_columns.py
index 79093e2..f3b9492 100644
--- a/pyxform/tests_v1/test_sheet_columns.py
+++ b/pyxform/tests_v1/test_sheet_columns.py
@@ -2,6 +2,10 @@ from pyxform.tests_v1.pyxform_test_case import PyxformTestCase
 
 
 class InvalidSurveyColumnsTests(PyxformTestCase):
+    """
+    Invalid survey column tests
+    """
+
     def test_missing_name(self):
         """
         every question needs a name (or alias of name)
@@ -51,7 +55,15 @@ class InvalidSurveyColumnsTests(PyxformTestCase):
 
 
 class InvalidChoiceSheetColumnsTests(PyxformTestCase):
+    """
+    Invalid choice sheet column tests
+    """
+
     def _simple_choice_ss(self, choice_sheet=None):
+        """
+        Return simple choices sheet
+        """
+
         if choice_sheet is None:
             choice_sheet = []
         return {'survey': [{'type': 'select_one l1',
@@ -60,6 +72,10 @@ class InvalidChoiceSheetColumnsTests(PyxformTestCase):
                 'choices': choice_sheet}
 
     def test_valid_choices_sheet_passes(self):
+        """
+        Test invalid choices sheet passes
+        """
+
         self.assertPyxformXform(
             name='valid_choices',
             ss_structure=self._simple_choice_ss([
@@ -73,6 +89,10 @@ class InvalidChoiceSheetColumnsTests(PyxformTestCase):
             )
 
     def test_invalid_choices_sheet_fails(self):
+        """
+        Test invalid choices sheet fails
+        """
+
         self.assertPyxformXform(
             name='missing_name',
             ss_structure=self._simple_choice_ss([
@@ -86,6 +106,10 @@ class InvalidChoiceSheetColumnsTests(PyxformTestCase):
             )
 
     def test_missing_list_name(self):
+        """
+        Test missing sheet name
+        """
+
         self.assertPyxformXform(
             name='missing_list_name',
             ss_structure=self._simple_choice_ss([
@@ -107,6 +131,10 @@ class InvalidChoiceSheetColumnsTests(PyxformTestCase):
 
 
 class AliasesTests(PyxformTestCase):
+    """
+    Aliases Tests
+    """
+
     def test_value_and_name(self):
         '''
         confirm that both 'name' and 'value' columns of choice list work
@@ -137,6 +165,8 @@ class AliasesTests(PyxformTestCase):
                     '<value>no</value>',
                     '</select1>',
                     ])
+
+
 ''' # uncomment when re-implemented
     # TODO: test that this fails for the correct reason
     def test_conflicting_aliased_values_raises_error(self):
diff --git a/pyxform/tests_v1/test_xmldata.py b/pyxform/tests_v1/test_xmldata.py
index d1d4895..05f517c 100644
--- a/pyxform/tests_v1/test_xmldata.py
+++ b/pyxform/tests_v1/test_xmldata.py
@@ -1,13 +1,12 @@
 from pyxform.errors import PyXFormError
 from pyxform.tests_v1.pyxform_test_case import PyxformTestCase
 from pyxform.tests_v1.pyxform_test_case import PyxformTestError
-try:
-    from unittest import skip
-except ImportError:
-    from unittest2 import skip
 
 
 class ExternalInstanceTests(PyxformTestCase):
+    """
+    External Instance Tests
+    """
 
     def test_can__output_single_external_xml_item(self):
         """Simplest possible example to include an external instance."""
@@ -35,7 +34,7 @@ class ExternalInstanceTests(PyxformTestCase):
                 """,
                 model__contains=[])
         # This is caught first by existing validation rule.
-        self.assertIn("There are two survey elements named 'mydata'",
+        self.assertIn("There are more than one survey elements named 'mydata'",
                       repr(ctx.exception))
 
     def test_can__use_unique_external_xml_in_same_section(self):
@@ -108,22 +107,22 @@ class ExternalInstanceTests(PyxformTestCase):
         with self.assertRaises(PyxformTestError) as ctx:
             self.assertPyxformXform(
                 md="""
-                | survey |                                      |      |       |
-                |        | type                                 | name | label | calculation  |
-                |        | begin group                          | g1   |       |              |
-                |        | xml-external                         | city |       |              |
-                |        | end group                            | g1   |       |              |
-                |        | xml-external                         | city |       |              |
-                |        | begin group                          | g2   |       |              |
-                |        | select_one_from_file cities.csv      | city | City  |              |
-                |        | end group                            | g2   |       |              |
-                |        | begin group                          | g3   |       |              |
-                |        | select_multiple_from_file cities.csv | city | City  |              |
-                |        | end group                            | g3   |       |              |
-                |        | begin group                          | g4   |       |              |
+                | survey |                                      |      |       |                                             |
+                |        | type                                 | name | label | calculation                                 |
+                |        | begin group                          | g1   |       |                                             |
+                |        | xml-external                         | city |       |                                             |
+                |        | end group                            | g1   |       |                                             |
+                |        | xml-external                         | city |       |                                             |
+                |        | begin group                          | g2   |       |                                             |
+                |        | select_one_from_file cities.csv      | city | City  |                                             |
+                |        | end group                            | g2   |       |                                             |
+                |        | begin group                          | g3   |       |                                             |
+                |        | select_multiple_from_file cities.csv | city | City  |                                             |
+                |        | end group                            | g3   |       |                                             |
+                |        | begin group                          | g4   |       |                                             |
                 |        | calculate                            | city | City  | pulldata('fruits', 'name', 'name', 'mango') |
-                |        | end group                            | g4   |       |              |
-                """,
+                |        | end group                            | g4   |       |                                             |
+                """,  # noqa
                 model__contains=[])
         self.assertIn("The name 'city' was found 2 time(s)",
                       repr(ctx.exception))
@@ -132,28 +131,28 @@ class ExternalInstanceTests(PyxformTestCase):
         """Unique instances with other sources present are OK."""
         self.assertPyxformXform(
             md="""
-            | survey  |                                      |       |       |              |
-            |         | type                                 | name  | label | calculation  | choice_filter |
-            |         | begin group                          | g1    |       | | |
-            |         | xml-external                         | city1 |       | | |
-            |         | note                                 | note1 | Note  | | |
-            |         | end group                            | g1    |       | | |
-            |         | begin group                          | g2    |       | | |
-            |         | select_one_from_file cities.csv      | city2 | City2 | | |
-            |         | end group                            | g2    |       | | |
-            |         | begin group                          | g3    |       | | |
-            |         | select_multiple_from_file cities.csv | city3 | City3 | | |
-            |         | end group                            | g3    |       | | |
-            |         | begin group                          | g4    |       | | |
-            |         | calculate                            | city4 | City4 | pulldata('fruits', 'name', 'name', 'mango') | |
-            |         | note                                 | note4 | Note  | | |
-            |         | end group                            | g4    |       | | |
-            |         | select_one states                    | test  | Test  | | true() |
-            | choices |                                      |       |       | | |
-            |         | list_name                            | name  | label | | |
-            |         | states                               | 1     | Pass  | | |
-            |         | states                               | 2     | Fail  | | |
-            """,
+            | survey  |                                      |       |       |                                             |               |
+            |         | type                                 | name  | label | calculation                                 | choice_filter |
+            |         | begin group                          | g1    |       |                                             |               |
+            |         | xml-external                         | city1 |       |                                             |               |
+            |         | note                                 | note1 | Note  |                                             |               |
+            |         | end group                            | g1    |       |                                             |               |
+            |         | begin group                          | g2    |       |                                             |               |
+            |         | select_one_from_file cities.csv      | city2 | City2 |                                             |               |
+            |         | end group                            | g2    |       |                                             |               |
+            |         | begin group                          | g3    |       |                                             |               |
+            |         | select_multiple_from_file cities.csv | city3 | City3 |                                             |               |
+            |         | end group                            | g3    |       |                                             |               |
+            |         | begin group                          | g4    |       |                                             |               |
+            |         | calculate                            | city4 | City4 | pulldata('fruits', 'name', 'name', 'mango') |               |
+            |         | note                                 | note4 | Note  |                                             |               |
+            |         | end group                            | g4    |       |                                             |               |
+            |         | select_one states                    | test  | Test  |                                             | true()        |
+            | choices |                                      |       |       |                                             |               |
+            |         | list_name                            | name  | label |                                             |               |
+            |         | states                               | 1     | Pass  |                                             |               |
+            |         | states                               | 2     | Fail  |                                             |               |
+            """,  # noqa
             model__contains=[
                 '<instance id="city1" src="jr://file/city1.xml">',
 """
@@ -181,22 +180,22 @@ class ExternalInstanceTests(PyxformTestCase):
         </root>
       </instance>
 """
-            ],
+            ],  # noqa
             run_odk_validate=True
         )
 
     def test_cannot__use_different_src_same_id__select_then_internal(self):
         """Duplicate instance from internal choices raises an error: #88."""
         md = """
-            | survey  |                                      |       |       |               |
-            |         | type                                 | name  | label | choice_filter |
-            |         | select_one_from_file states.csv      | state | State |               |
-            |         | select_one states                    | test  | Test  | state=/select_from_file_test/state |
-            | choices |                                      |       |       |               |
-            |         | list_name                            | name  | label |               |
-            |         | states                               | 1     | Pass  |               |
-            |         | states                               | 2     | Fail  |               |
-            """
+            | survey  |                                 |       |       |                                    |
+            |         | type                            | name  | label | choice_filter                      |
+            |         | select_one_from_file states.csv | state | State |                                    |
+            |         | select_one states               | test  | Test  | state=/select_from_file_test/state |
+            | choices |                                 |       |       |                                    |
+            |         | list_name                       | name  | label |                                    |
+            |         | states                          | 1     | Pass  |                                    |
+            |         | states                          | 2     | Fail  |                                    |
+            """  # noqa
         with self.assertRaises(PyXFormError) as ctx:
             survey = self.md_to_pyxform_survey(md_raw=md)
             survey._to_pretty_xml()
@@ -209,45 +208,49 @@ class ExternalInstanceTests(PyxformTestCase):
         )
 
     def test_cannot__use_different_src_same_id__external_then_pulldata(self):
-        """Duplicate instance from pulldata after xml-external raises an error."""
+        """
+        Duplicate instance from pulldata after xml-external raises an error.
+        """
         md = """
-            | survey |                                      |        |       |              |
-            |        | type                                 | name   | label | calculation  |
-            |        | begin group                          | g1     |       |              |
-            |        | xml-external                         | fruits |       |              |
-            |        | calculate                            | f_csv  | City  | pulldata('fruits', 'name', 'name', 'mango') |
-            |        | note                                 | note   | Fruity! ${f_csv} |   |
-            |        | end group                            | g1     |       |              |
-            """
+            | survey |              |        |                  |                                             |
+            |        | type         | name   | label            | calculation                                 |
+            |        | begin group  | g1     |                  |                                             |
+            |        | xml-external | fruits |                  |                                             |
+            |        | calculate    | f_csv  | City             | pulldata('fruits', 'name', 'name', 'mango') |
+            |        | note         | note   | Fruity! ${f_csv} |                                             |
+            |        | end group    | g1     |                  |                                             |
+            """  # noqa
         with self.assertRaises(PyXFormError) as ctx:
             survey = self.md_to_pyxform_survey(md_raw=md)
             survey._to_pretty_xml()
         self.assertIn(
             "Instance name: 'fruits', "
             "Existing type: 'external', Existing URI: 'jr://file/fruits.xml', "
-            "Duplicate type: 'pulldata', Duplicate URI: 'jr://file-csv/fruits.csv', "
+            "Duplicate type: 'pulldata', Duplicate URI: 'jr://file-csv/fruits.csv', "  # noqa
             "Duplicate context: '[type: group, name: g1]'.",
             repr(ctx.exception)
         )
 
     def test_cannot__use_different_src_same_id__pulldata_then_external(self):
-        """Duplicate instance from xml-external after pulldata raises an error."""
+        """
+        Duplicate instance from xml-external after pulldata raises an error.
+        """
         md = """
-            | survey |                                      |        |       |              |
-            |        | type                                 | name   | label | calculation  |
-            |        | begin group                          | g1     |       |              |
-            |        | calculate                            | f_csv  | City  | pulldata('fruits', 'name', 'name', 'mango') |
-            |        | xml-external                         | fruits |       |              |
-            |        | note                                 | note   | Fruity! ${f_csv} |   |
-            |        | end group                            | g1     |       |              |
-            """
+            | survey |              |        |                  |                                             |
+            |        | type         | name   | label            | calculation                                 |
+            |        | begin group  | g1     |                  |                                             |
+            |        | calculate    | f_csv  | City             | pulldata('fruits', 'name', 'name', 'mango') |
+            |        | xml-external | fruits |                  |                                             |
+            |        | note         | note   | Fruity! ${f_csv} |                                             |
+            |        | end group    | g1     |                  |                                             |
+            """  # noqa
         with self.assertRaises(PyXFormError) as ctx:
             survey = self.md_to_pyxform_survey(md_raw=md)
             survey._to_pretty_xml()
         self.assertIn(
             "Instance name: 'fruits', "
-            "Existing type: 'pulldata', Existing URI: 'jr://file-csv/fruits.csv', "
-            "Duplicate type: 'external', Duplicate URI: 'jr://file/fruits.xml', "
+            "Existing type: 'pulldata', Existing URI: 'jr://file-csv/fruits.csv', "  # noqa
+            "Duplicate type: 'external', Duplicate URI: 'jr://file/fruits.xml', "  # noqa
             "Duplicate context: '[type: group, name: g1]'.",
             repr(ctx.exception)
         )
@@ -255,15 +258,15 @@ class ExternalInstanceTests(PyxformTestCase):
     def test_can__reuse_csv__selects_then_pulldata(self):
         """Re-using the same csv external data source id and URI is OK."""
         md = """
-            | survey |                                              |        |                                    |             |
-            |        | type                                         | name   | label                              | calculation |
-            |        | select_multiple_from_file pain_locations.csv | plocs  | Locations of pain this week.       | |
-            |        | select_one_from_file pain_locations.csv      | pweek  | Location of worst pain this week.  | |
-            |        | select_one_from_file pain_locations.csv      | pmonth | Location of worst pain this month. | |
-            |        | select_one_from_file pain_locations.csv      | pyear  | Location of worst pain this year.  | |
-            |        | calculate                                    | f_csv  | pd | pulldata('pain_locations', 'name', 'name', 'arm') |
-            |        | note                                         | note   | Arm ${f_csv} | |
-            """
+            | survey |                                              |        |                                    |                                                   |
+            |        | type                                         | name   | label                              | calculation                                       |
+            |        | select_multiple_from_file pain_locations.csv | plocs  | Locations of pain this week.       |                                                   |
+            |        | select_one_from_file pain_locations.csv      | pweek  | Location of worst pain this week.  |                                                   |
+            |        | select_one_from_file pain_locations.csv      | pmonth | Location of worst pain this month. |                                                   |
+            |        | select_one_from_file pain_locations.csv      | pyear  | Location of worst pain this year.  |                                                   |
+            |        | calculate                                    | f_csv  | pd                                 | pulldata('pain_locations', 'name', 'name', 'arm') |
+            |        | note                                         | note   | Arm ${f_csv}                       |                                                   |
+            """  # noqa
         expected = \
 """
       <instance id="pain_locations" src="jr://file-csv/pain_locations.csv">
@@ -274,7 +277,7 @@ class ExternalInstanceTests(PyxformTestCase):
           </item>
         </root>
       </instance>
-"""
+"""  # noqa
         self.assertPyxformXform(
             md=md, model__contains=[expected], run_odk_validate=True)
         survey = self.md_to_pyxform_survey(md_raw=md)
@@ -284,15 +287,15 @@ class ExternalInstanceTests(PyxformTestCase):
     def test_can__reuse_csv__pulldata_then_selects(self):
         """Re-using the same csv external data source id and URI is OK."""
         md = """
-            | survey |                                              |        |       |             |
-            |        | type                                         | name   | label | calculation |
-            |        | calculate                                    | f_csv  | pd | pulldata('pain_locations', 'name', 'name', 'arm') |
-            |        | note                                         | note   | Arm ${f_csv} | |
-            |        | select_multiple_from_file pain_locations.csv | plocs  | Locations of pain this week.       | |
-            |        | select_one_from_file pain_locations.csv      | pweek  | Location of worst pain this week.  | |
-            |        | select_one_from_file pain_locations.csv      | pmonth | Location of worst pain this month. | |
-            |        | select_one_from_file pain_locations.csv      | pyear  | Location of worst pain this year.  | |
-            """
+            | survey |                                              |        |                                    |                                                   |
+            |        | type                                         | name   | label                              | calculation                                       |
+            |        | calculate                                    | f_csv  | pd                                 | pulldata('pain_locations', 'name', 'name', 'arm') |
+            |        | note                                         | note   | Arm ${f_csv}                       |                                                   |
+            |        | select_multiple_from_file pain_locations.csv | plocs  | Locations of pain this week.       |                                                   |
+            |        | select_one_from_file pain_locations.csv      | pweek  | Location of worst pain this week.  |                                                   |
+            |        | select_one_from_file pain_locations.csv      | pmonth | Location of worst pain this month. |                                                   |
+            |        | select_one_from_file pain_locations.csv      | pyear  | Location of worst pain this year.  |                                                   |
+            """  # noqa
         expected = \
 """
       <instance id="pain_locations" src="jr://file-csv/pain_locations.csv">
@@ -303,21 +306,21 @@ class ExternalInstanceTests(PyxformTestCase):
           </item>
         </root>
       </instance>
-"""
+"""  # noqa
         self.assertPyxformXform(
             md=md, model__contains=[expected], run_odk_validate=True)
 
     def test_can__reuse_xml__selects_then_external(self):
         """Re-using the same xml external data source id and URI is OK."""
         md = """
-            | survey |                                              |        |        |
-            |        | type                                         | name   | label  | 
-            |        | select_multiple_from_file pain_locations.xml | plocs  | Locations of pain this week.       |
-            |        | select_one_from_file pain_locations.xml      | pweek  | Location of worst pain this week.  |
-            |        | select_one_from_file pain_locations.xml      | pmonth | Location of worst pain this month. |
-            |        | select_one_from_file pain_locations.xml      | pyear  | Location of worst pain this year.  |
-            |        | xml-external                                 | pain_locations  | |
-            """
+            | survey |                                              |                |                                    |
+            |        | type                                         | name           | label                              |
+            |        | select_multiple_from_file pain_locations.xml | plocs          | Locations of pain this week.       |
+            |        | select_one_from_file pain_locations.xml      | pweek          | Location of worst pain this week.  |
+            |        | select_one_from_file pain_locations.xml      | pmonth         | Location of worst pain this month. |
+            |        | select_one_from_file pain_locations.xml      | pyear          | Location of worst pain this year.  |
+            |        | xml-external                                 | pain_locations |                                    |
+            """  # noqa
         expected = \
 """
       <instance id="pain_locations" src="jr://file/pain_locations.xml">
@@ -328,7 +331,7 @@ class ExternalInstanceTests(PyxformTestCase):
           </item>
         </root>
       </instance>
-"""
+"""  # noqa
         survey = self.md_to_pyxform_survey(md_raw=md)
         xml = survey._to_pretty_xml()
         self.assertEqual(1, xml.count(expected))
@@ -336,14 +339,14 @@ class ExternalInstanceTests(PyxformTestCase):
     def test_can__reuse_xml__external_then_selects(self):
         """Re-using the same xml external data source id and URI is OK."""
         md = """
-            | survey |                                              |        |        |
-            |        | type                                         | name   | label  | 
-            |        | xml-external                                 | pain_locations  | |
-            |        | select_multiple_from_file pain_locations.xml | plocs  | Locations of pain this week.       |
-            |        | select_one_from_file pain_locations.xml      | pweek  | Location of worst pain this week.  |
-            |        | select_one_from_file pain_locations.xml      | pmonth | Location of worst pain this month. |
-            |        | select_one_from_file pain_locations.xml      | pyear  | Location of worst pain this year.  |
-            """
+            | survey |                                              |                |                                    |
+            |        | type                                         | name           | label                              |
+            |        | xml-external                                 | pain_locations |                                    |
+            |        | select_multiple_from_file pain_locations.xml | plocs          | Locations of pain this week.       |
+            |        | select_one_from_file pain_locations.xml      | pweek          | Location of worst pain this week.  |
+            |        | select_one_from_file pain_locations.xml      | pmonth         | Location of worst pain this month. |
+            |        | select_one_from_file pain_locations.xml      | pyear          | Location of worst pain this year.  |
+            """  # noqa
         expected = \
 """
       <instance id="pain_locations" src="jr://file/pain_locations.xml">
@@ -354,7 +357,7 @@ class ExternalInstanceTests(PyxformTestCase):
           </item>
         </root>
       </instance>
-"""
+"""  # noqa
         self.assertPyxformXform(
             md=md, model__contains=[expected], run_odk_validate=True)
         survey = self.md_to_pyxform_survey(md_raw=md)