Extract and decode feature names in one try/except

This changes the logic for extracting and decoding the name from a
feature so that Exceptions are caught more selectively and the right
errors are reported to the user.

Previously, a UnicodeDecodeError could occur when the name field
was read (because internally, the GDAL api uses force_text on
the output) and the code would catch it but instead report that the
name field didn't exist. This was confusing on files with encoding
errors.

Closes #168

mapit/management/commands/mapit_import.py

@@ -215,16 +215,15 @@ def verbose(*args):
                 name = override_name
             else:
                 try:
-                    name = feat[name_field].value
-                except:
-                    choices = ', '.join(layer.fields)
-                    raise CommandError(
-                        "Could not find name using name field '%s' - should it be something else? "
-                        "It will be one of these: %s. Specify which with --name_field" % (name_field, choices))
-                try:
+                    name = feat.get(name_field)
+                    if name is None:
+                        choices = ', '.join(layer.fields)
+                        raise CommandError(
+                            "Could not find name using name field '%s' - should it be something else? "
+                            "It will be one of these: %s. Specify which with --name_field" % (name_field, choices))
                     if not isinstance(name, six.text_type):
                         name = name.decode(encoding)
-                except:
+                except UnicodeDecodeError:
                     raise CommandError(
                         "Could not decode name using encoding '%s' - is it in another encoding? "
                         "Specify one with --encoding" % encoding)

mapit/tests/fixtures/bad_encoding.prj

@@ -0,0 +1 @@
+PROJCS["British_National_Grid",GEOGCS["GCS_OSGB_1936",DATUM["D_OSGB_1936",SPHEROID["Airy_1830",6377563.396,299.3249646]],PRIMEM["Greenwich",0.0],UNIT["Degree",0.0174532925199433]],PROJECTION["Transverse_Mercator"],PARAMETER["False_Easting",400000.0],PARAMETER["False_Northing",-100000.0],PARAMETER["Central_Meridian",-2.0],PARAMETER["Scale_Factor",0.9996012717],PARAMETER["Latitude_Of_Origin",49.0],UNIT["Meter",1.0],AUTHORITY["EPSG",27700]]

mapit/tests/test_import_commands.py

@@ -2,6 +2,7 @@
 
 from django.test import TestCase
 from django.core.management import call_command
+from django.core.management.base import CommandError
 from django.conf import settings
 from django.utils.six import StringIO
 
@@ -11,35 +12,38 @@
 class MapitImportTest(TestCase):
     """Test the mapit_import management command"""
 
-    def test_loads_kml_files(self):
-        # Assert no areas in db before
-        self.assertEqual(Area.objects.count(), 0)
-
+    def setUp(self):
         # Create a country, generation, area type and a name type for the
         # areas we'll load in from the KML file
-        england = Country.objects.create(code="E", name="England")
-        generation = Generation.objects.create(
+        self.england = Country.objects.create(code="E", name="England")
+        self.generation = Generation.objects.create(
             active=True,
             description="Test generation",
         )
-        big_type = Type.objects.create(
+        self.big_type = Type.objects.create(
             code="BIG",
             description="A large test area",
         )
-        os_name_type = NameType.objects.create(
+        self.os_name_type = NameType.objects.create(
             code='O',
             description="Ordnance Survey name",
         )
 
+    def test_loads_kml_files(self):
+        # Assert no areas in db before
+        self.assertEqual(Area.objects.count(), 0)
+
+
+
         # Load in the KML file
         fixtures_dir = os.path.join(settings.BASE_DIR, 'mapit', 'tests', 'fixtures')
         call_command(
             'mapit_import',
             os.path.join(fixtures_dir, 'example.kml'),
-            country_code=england.code,
-            generation_id=generation.id,
-            area_type_code=big_type.code,
-            name_type_code=os_name_type.code,
+            country_code=self.england.code,
+            generation_id=self.generation.id,
+            area_type_code=self.big_type.code,
+            name_type_code=self.os_name_type.code,
             commit=True,
             # These keep the commands quiet, comment out if you're debugging
             stderr=StringIO(),
@@ -52,7 +56,31 @@ def test_loads_kml_files(self):
         # Check it loaded the data properly
         area = Area.objects.all()[0]
         self.assertEqual(area.name, "London")
-        self.assertEqual(area.country.name, england.name)
-        self.assertEqual(area.generation_low.id, generation.id)
-        self.assertEqual(area.generation_high.id, generation.id)
+        self.assertEqual(area.country.name, self.england.name)
+        self.assertEqual(area.generation_low.id, self.generation.id)
+        self.assertEqual(area.generation_high.id, self.generation.id)
         self.assertEqual(area.type.code, "BIG")
+
+    def test_reports_encoding_errors(self):
+        # Load in a badly encoded file (it contains Windows charset encoded
+        # apostrophe's, but reports itself as being UTF-8)
+        fixtures_dir = os.path.join(settings.BASE_DIR, 'mapit', 'tests', 'fixtures')
+        with self.assertRaises(CommandError) as context:
+            call_command(
+                'mapit_import',
+                os.path.join(fixtures_dir, 'bad_encoding.shp'),
+                country_code=self.england.code,
+                generation_id=self.generation.id,
+                area_type_code=self.big_type.code,
+                name_type_code=self.os_name_type.code,
+                # If we commit we see other errors in the sample fixture, and
+                # there's no need to for this test
+                commit=False,
+                # These keep the commands quiet, comment out if you're debugging
+                stderr=StringIO(),
+                stdout=StringIO(),
+            )
+            # Previously, this would have been a message about not being able
+            # to find the name field.
+            expected_message = "Could not decode name using encoding 'utf-8' - is it in another encoding? \nSpecify one with --encoding"
+            self.assertEqual(context.exception.message, expected_message)