Repository: mysociety/mapit (language: python)

Issue:
Add lookup of areas near a point
Our `/point` endpoint (areas covering a point) could also be described as "all the areas within 0m of a point", so extending that with a parameter to look up areas within Nm of a point could be useful. Or it could be added to the `/nearest` endpoint.


Discussion / hints:

Fix patch:
diff --git a/conf/general.yml-example b/conf/general.yml-example
index 09d3211c..d598590d 100644
--- a/conf/general.yml-example
+++ b/conf/general.yml-example
@@ -18,6 +18,10 @@ COUNTRY: 'GB'
 # Optional, defaults to 4326.
 AREA_SRID: 27700
 
+# Set this to the maximum distance (in AREA_SRID units) allowed for the within
+# parameter to the point call. Optional, defaults to 0 (off).
+WITHIN_MAXIMUM: 0
+
 # A secret key for this particular Django installation.
 # Set this to a random string -- the longer, the better.
 DJANGO_SECRET_KEY: 'gu^&xc)hoibh3x&s+9009jbn4d$!nq0lz+syx-^x8%z24!kfs4'
diff --git a/mapit/context_processors.py b/mapit/context_processors.py
index 3d9b1a8f..81b16e02 100644
--- a/mapit/context_processors.py
+++ b/mapit/context_processors.py
@@ -4,6 +4,8 @@
 def country(request):
     return {
         'country': settings.MAPIT_COUNTRY,
+        'area_srid_units': 'degrees' if settings.MAPIT_AREA_SRID == 4326 else 'metres',
+        'within_maximum': settings.MAPIT_WITHIN_MAXIMUM,
         'postcodes_available': settings.POSTCODES_AVAILABLE,
         'partial_postcodes_available': settings.PARTIAL_POSTCODES_AVAILABLE,
     }
diff --git a/mapit/management/commands/mapit_delete_areas_from_new_generation.py b/mapit/management/commands/mapit_delete_areas_from_new_generation.py
index a545fea9..7227d808 100644
--- a/mapit/management/commands/mapit_delete_areas_from_new_generation.py
+++ b/mapit/management/commands/mapit_delete_areas_from_new_generation.py
@@ -1,8 +1,6 @@
 # This script deletes all the areas from the new generation (i.e. the
 # most recent inactive one).
 
-from __future__ import print_function
-
 from django.core.management.base import BaseCommand, CommandError
 from mapit.models import Generation, Area
 
@@ -26,7 +24,7 @@ def handle(self, **options):
 
         for area in Area.objects.filter(generation_low__lte=new, generation_high__gte=new):
 
-            print("Considering", area)
+            self.stdout.write("Considering %s" % area)
 
             g_low = area.generation_low
             g_high = area.generation_high
@@ -37,22 +35,22 @@ def handle(self, **options):
                 raise Exception("area.generation_high was " + g_high + ", which no longer exists!")
 
             if area.generation_low == new and area.generation_high == new:
-                print("  ... only exists in", new, "so will delete")
+                self.stdout.write("  ... only exists in %s so will delete" % new)
                 if options['commit']:
                     area.delete()
-                    print("  ... deleted.")
+                    self.stdout.write("  ... deleted.")
                 else:
-                    print("  ... not deleting, since --commit wasn't specified")
+                    self.stdout.write("  ... not deleting, since --commit wasn't specified")
             elif area.generation_low.id < new.id and area.generation_high == new:
-                print(
-                    "  ... still exists in an earlier generation, so lowering generation_high to",
+                self.stdout.write(
+                    "  ... still exists in an earlier generation, so lowering generation_high to %s" %
                     previous_generation)
                 area.generation_high = previous_generation
                 if options['commit']:
                     area.save()
-                    print("  ... lowered.")
+                    self.stdout.write("  ... lowered.")
                 else:
-                    print("  ... not lowering, since --commit wasn't specified")
+                    self.stdout.write("  ... not lowering, since --commit wasn't specified")
 
             elif area.generation_high.id > new.id:
                 # This should never happen - it'd mean the
diff --git a/mapit/management/commands/mapit_generation_activate.py b/mapit/management/commands/mapit_generation_activate.py
index aeec6351..48b6c1c3 100644
--- a/mapit/management/commands/mapit_generation_activate.py
+++ b/mapit/management/commands/mapit_generation_activate.py
@@ -1,6 +1,6 @@
 # This script activates the currently inactive generation.
 
-from django.core.management.base import BaseCommand
+from django.core.management.base import BaseCommand, CommandError
 from mapit.models import Generation
 
 
@@ -13,11 +13,11 @@ def add_arguments(self, parser):
     def handle(self, **options):
         new = Generation.objects.new()
         if not new:
-            raise Exception("You do not have an inactive generation to activate")
+            raise CommandError("You do not have an inactive generation to activate")
 
         new.active = True
         if options['commit']:
             new.save()
-            print("%s - activated" % new)
+            self.stdout.write("%s - activated" % new)
         else:
-            print("%s - not activated, dry run" % new)
+            self.stdout.write("%s - not activated, dry run" % new)
diff --git a/mapit/management/commands/mapit_generation_create.py b/mapit/management/commands/mapit_generation_create.py
index 93d56eda..ad2d883d 100644
--- a/mapit/management/commands/mapit_generation_create.py
+++ b/mapit/management/commands/mapit_generation_create.py
@@ -1,7 +1,7 @@
 # This script is used to create a new inactive generation for
 # inputting new boundaries of some sort.
 
-from django.core.management.base import BaseCommand
+from django.core.management.base import BaseCommand, CommandError
 from mapit.models import Generation
 
 
@@ -15,15 +15,15 @@ def add_arguments(self, parser):
     def handle(self, **options):
         new_generation = Generation.objects.new()
         if new_generation:
-            raise Exception("You already have an inactive generation")
+            raise CommandError("You already have an inactive generation")
 
         if not options['desc']:
-            raise Exception("You must specify a generation description")
+            raise CommandError("You must specify a generation description")
 
         g = Generation(description=options['desc'])
-        print("Creating generation...")
+        self.stdout.write("Creating generation...")
         if options['commit']:
             g.save()
-            print("...saved: %s" % g)
+            self.stdout.write("...saved: %s" % g)
         else:
-            print("...not saving, dry run")
+            self.stdout.write("...not saving, dry run")
diff --git a/mapit/management/commands/mapit_generation_deactivate.py b/mapit/management/commands/mapit_generation_deactivate.py
index 53b932d7..0912e9f2 100644
--- a/mapit/management/commands/mapit_generation_deactivate.py
+++ b/mapit/management/commands/mapit_generation_deactivate.py
@@ -25,6 +25,6 @@ def handle(self, **options):
         generation_to_deactivate.active = False
         if options['commit']:
             generation_to_deactivate.save()
-            print("%s - deactivated" % generation_to_deactivate)
+            self.stdout.write("%s - deactivated" % generation_to_deactivate)
         else:
-            print("%s - not deactivated, dry run" % generation_to_deactivate)
+            self.stdout.write("%s - not deactivated, dry run" % generation_to_deactivate)
diff --git a/mapit/templates/mapit/api/point.html b/mapit/templates/mapit/api/point.html
index 27155110..7536207f 100644
--- a/mapit/templates/mapit/api/point.html
+++ b/mapit/templates/mapit/api/point.html
@@ -63,6 +63,13 @@ <h3>{% blocktrans trimmed %}
                                 <i>country</i>, to restrict results to areas with particular country codes (multiple country codes separated by commas).
                             {% endblocktrans %}
                         </li>
+                      {% if within_maximum %}
+                        <li>
+                            {% blocktrans trimmed %}
+                                <i>within</i>, to show results within the specified distance (up to {{ within_maximum }} {{ area_srid_units }}) of the point, not just the point itself (not /box).
+                            {% endblocktrans %}
+                        </li>
+                      {% endif %}
                     </ul>
                 </dd>
 
diff --git a/mapit/views/areas.py b/mapit/views/areas.py
index 7bca20a9..105de6d0 100644
--- a/mapit/views/areas.py
+++ b/mapit/views/areas.py
@@ -394,8 +394,18 @@ def areas_by_point(request, srid, x, y, bb=False, format=''):
 
     q = query_args(request, format)
 
+    max_within = getattr(settings, 'MAPIT_WITHIN_MAXIMUM', 0)
+    try:
+        within = float(request.GET.get('within', 0))
+        if within < 0 or within > max_within:
+            raise ValueError
+    except ValueError:
+        raise ViewException(format, _('Bad "within" parameter specified'), 400)
+
     if method == 'box':
         q &= Q(polygons__polygon__bbcontains=location)
+    elif within:
+        q &= Q(polygons__polygon__dwithin=(location, within))
     else:
         q &= Q(polygons__polygon__contains=location)
     areas = Area.objects.filter(q).distinct()
diff --git a/project/settings.py b/project/settings.py
index 45c86219..26ad8a62 100644
--- a/project/settings.py
+++ b/project/settings.py
@@ -22,6 +22,12 @@
 # WGS84. Optional, defaults to 4326.
 MAPIT_AREA_SRID = int(config.get('AREA_SRID', 4326))
 
+# Set this to the maximum distance (in AREA_SRID units) allowed for the within
+# parameter to the point call. Optional, defaults to 0 (off).
+MAPIT_WITHIN_MAXIMUM = float(config.get('WITHIN_MAXIMUM', 0))
+if MAPIT_WITHIN_MAXIMUM.is_integer():
+    MAPIT_WITHIN_MAXIMUM = int(MAPIT_WITHIN_MAXIMUM)
+
 # Country is currently one of GB, NO, IT, KE, SA, or ZA.
 # Optional; country specific things won't happen if not set.
 MAPIT_COUNTRY = config.get('COUNTRY', '')


Test patch:
diff --git a/mapit/tests/test_generation_commands.py b/mapit/tests/test_generation_commands.py
index ba745ce3..32d4af8a 100644
--- a/mapit/tests/test_generation_commands.py
+++ b/mapit/tests/test_generation_commands.py
@@ -5,7 +5,106 @@
 from ..models import Area, Country, Generation, Type
 
 
-class GenerationCommandTests(TestCase):
+class GenerationCreateCommandTests(TestCase):
+    """Tests for commands that create MapIt generations"""
+
+    def test_already_new(self):
+        Generation.objects.create(active=False, description="One inactive generation")
+        with assertRaisesRegex(self, CommandError, r'already have an inactive'):
+            call_command('mapit_generation_create')
+
+    def test_no_description(self):
+        with assertRaisesRegex(self, CommandError, r'must specify a generation description'):
+            call_command('mapit_generation_create')
+
+    def test_no_commit(self):
+        stdout = StringIO()
+        call_command('mapit_generation_create', desc='An inactive generation', stdout=stdout)
+        self.assertEqual(
+            stdout.getvalue(),
+            'Creating generation...\n...not saving, dry run\n'
+        )
+
+    def test_creation(self):
+        stdout = StringIO()
+        call_command('mapit_generation_create', commit=True, desc='An inactive generation', stdout=stdout)
+        gen = Generation.objects.first().id
+        self.assertEqual(
+            stdout.getvalue(),
+            'Creating generation...\n...saved: Generation %d (inactive)\n' % gen
+        )
+
+
+class GenerationActivateCommandTests(TestCase):
+    """Tests for commands that activate MapIt generations"""
+
+    def test_no_new(self):
+        with assertRaisesRegex(self, CommandError, r'do not have an inactive'):
+            call_command('mapit_generation_activate')
+
+    def test_no_commit(self):
+        g = Generation.objects.create(active=False, description="One inactive generation")
+        stdout = StringIO()
+        call_command('mapit_generation_activate', stderr=StringIO(), stdout=stdout)
+        self.assertEqual(
+            stdout.getvalue(),
+            'Generation %s (active) - not activated, dry run\n' % g.id
+        )
+
+    def test_activation(self):
+        g = Generation.objects.create(active=False, description="One inactive generation")
+        stdout = StringIO()
+        call_command('mapit_generation_activate', commit=True, stderr=StringIO(), stdout=stdout)
+        self.assertEqual(
+            stdout.getvalue(),
+            'Generation %s (active) - activated\n' % g.id
+        )
+
+
+class GenerationDeactivateCommandTests(TestCase):
+    """Tests for commands that deactivate MapIt generations"""
+
+    def test_bad_id(self):
+        with assertRaisesRegex(self, Generation.DoesNotExist, 'does not exist'):
+            call_command('mapit_generation_deactivate', 0)
+
+    def test_inactive(self):
+        g = Generation.objects.create(active=False, description="One inactive generation")
+        with assertRaisesRegex(self, CommandError, r"wasn't active"):
+            call_command('mapit_generation_deactivate', g.id)
+
+    def test_only_active(self):
+        g = Generation.objects.create(active=True, description="One active generation")
+        with assertRaisesRegex(self, CommandError, r'the only active generation'):
+            call_command('mapit_generation_deactivate', g.id)
+
+    def test_no_commit(self):
+        Generation.objects.create(active=True, description="One active generation")
+        g2 = Generation.objects.create(active=True, description="Two active generation")
+        stdout = StringIO()
+        call_command(
+            'mapit_generation_deactivate',
+            g2.id, stderr=StringIO(), stdout=stdout
+        )
+        self.assertEqual(
+            stdout.getvalue(),
+            'Generation %s (inactive) - not deactivated, dry run\n' % g2.id
+        )
+
+    def test_activation(self):
+        g = Generation.objects.create(active=True, description="One active generation")
+        stdout = StringIO()
+        call_command(
+            'mapit_generation_deactivate',
+            g.id, force=True, commit=True, stderr=StringIO(), stdout=stdout
+        )
+        self.assertEqual(
+            stdout.getvalue(),
+            'Generation %s (inactive) - deactivated\n' % g.id
+        )
+
+
+class GenerationRaiseCommandTests(TestCase):
     """Tests for commands that manipulate MapIt generations"""
 
     def test_no_generations(self):
@@ -180,3 +279,35 @@ def test_area_type_restriction(self):
 
         self.assertEqual(area_wmc.generation_high, inactive)
         self.assertEqual(area_uta.generation_high, current)
+
+
+class GenerationDeleteAreasCommandTests(TestCase):
+    """Tests for command that removes areas from a MapIt generations"""
+
+    def test_no_active_generations(self):
+        Generation.objects.create(active=True, description="One active generation")
+        with assertRaisesRegex(self, CommandError, r'no new inactive generation'):
+            call_command('mapit_delete_areas_from_new_generation')
+
+    def test_delete_areas(self):
+        cur = Generation.objects.create(active=True, description="One active generation")
+        new = Generation.objects.create(active=False, description="One inactive generation")
+        area_type = Type.objects.create(code='VIL', description='Villages in Trumptonshire')
+        area_trumpton = Area.objects.create(
+            name='Trumpton', generation_low=cur, generation_high=new, type=area_type)
+        area_chigley = Area.objects.create(
+            name='Chigley', generation_low=new, generation_high=new, type=area_type)
+
+        call_command(
+            'mapit_delete_areas_from_new_generation',
+            commit=True,
+            stderr=StringIO(),
+            stdout=StringIO(),
+        )
+
+        # The old areas will still be cached by the ORM, so refetch
+        area_trumpton = Area.objects.get(pk=area_trumpton.id)
+        self.assertEqual(area_trumpton.generation_low, cur)
+        self.assertEqual(area_trumpton.generation_high, cur)
+        with assertRaisesRegex(self, Area.DoesNotExist, 'does not exist'):
+            area_chigley = Area.objects.get(pk=area_chigley.id)
diff --git a/mapit/tests/test_views.py b/mapit/tests/test_views.py
index 8c7fae7b..2a083e92 100644
--- a/mapit/tests/test_views.py
+++ b/mapit/tests/test_views.py
@@ -1,7 +1,7 @@
 import json
 import unittest
 
-from django.test import TestCase
+from django.test import TestCase, override_settings
 from django.conf import settings
 from django.contrib.gis.geos import Polygon, Point
 
@@ -84,6 +84,34 @@ def test_areas_by_point(self):
             set((x.id for x in (self.big_area, self.small_area_1)))
             )
 
+    @override_settings(MAPIT_WITHIN_MAXIMUM=1000)
+    def test_areas_by_point_within(self):
+        url = '/point/4326/-4.001,51.json?within=Bad'
+        response = self.client.get(url, HTTP_ACCEPT_ENCODING='gzip')
+        content = get_content(response)
+        self.assertEqual(content, {'code': 400, 'error': 'Bad "within" parameter specified'})
+
+        # (51,-4.001) is about 70m from (51,-4)
+        url = '/point/4326/-4.001,51.json'
+        response = self.client.get(url, HTTP_ACCEPT_ENCODING='gzip')
+        content = get_content(response)
+        self.assertEqual(
+            set((int(x) for x in content.keys())),
+            set([self.big_area.id])
+            )
+
+        # Want this to pass whatever the SRID is set to
+        pt1 = Point(-4.001, 51, srid=4326).transform(settings.MAPIT_AREA_SRID, clone=True)
+        pt2 = Point(-4, 51, srid=4326).transform(settings.MAPIT_AREA_SRID, clone=True)
+        dist = pt1.distance(pt2)
+        url = '/point/4326/-4.001,51.json?within=%f' % (dist * 2)
+        response = self.client.get(url, HTTP_ACCEPT_ENCODING='gzip')
+        content = get_content(response)
+        self.assertEqual(
+            set((int(x) for x in content.keys())),
+            set((x.id for x in (self.big_area, self.small_area_1)))
+            )
+
     def test_front_page(self):
         response = self.client.get('/')
         self.assertContains(response, 'MapIt')