Require PostGIS and always enable "findhelp" app. (#1982)

In order to support determining what county a user's court is in for EvictionFreeNY.org, we'll need PostGIS.  Since this is necessary functionality, we'll need to _require_ PostGIS.  Since it will no longer be optional, this obviates the need for the `ENABLE_FINDHELP` setting introduced way back in #443.

Also, I had a problem when trying to change the protocol of `DATABASE_URL` on our staging instance--apparently Heroku doesn't like it when we try to change that setting, since it manages the database for us.  I found a [stack overflow post][1] that works around it, but it involved a lot of tomfoolery so I decided to instead just rewrite the Database URL at runtime to use `postgis://` if it was already `postgres://`.  A side benefit of this is that we won't need to change any environment variables on production or staging to have this PR take effect.

Finally, this also runs `manage.py loadfindhelpdata` whenever a new deployment is triggered, and as part of `update.sh`, so that we don't have to worry about manually running it whenever we add/change our built-in shapefile data.

## Breaking changes

Note that something annoying about our `ENABLE_FINDHELP` setting was that **even if findhelp was disabled, the database still regarded its migrations as having been applied**.  This means that all existing local deployments need to run the following command in order to have their findhelp app's migrations actually be copacetic:

    python manage.py migrate findhelp zero --fake

After that, developers should run `bash docker-update.sh` (or `bash update.sh` if they're not using Docker).

[1]: https://stackoverflow.com/a/35704410/2422398

.circleci/config.yml

@@ -13,7 +13,6 @@ jobs:
           PIPENV_VENV_IN_PROJECT: true
           DEBUG: yup
           ENABLE_WEBPACK_CONTENT_HASH: yup
-          ENABLE_FINDHELP: yup
           GIT_LFS_SKIP_SMUDGE: 1
     steps:
       - checkout

README.md

@@ -19,7 +19,8 @@ everything up without Docker, read on!
 
 You'll need Python 3.8.2 and [pipenv][], as well as Node 12, yarn, and
 [Git Large File Storage (LFS)][git-lfs]. You will also need to
-set up Postgres version 10 or later.
+set up Postgres version 10 or later, and it will need the PostGIS
+extension installed.
 
 If you didn't have Git LFS installed before cloning the repository,
 you can obtain the repository's large files by running `git lfs pull`.

conftest.py

@@ -17,19 +17,13 @@
 
 from users.tests.factories import UserFactory
 from project.schema import schema
-from project.settings import env
 from nycha.tests.fixtures import load_nycha_csv_data
 
 
 BASE_DIR = Path(__file__).parent.resolve()
 
 STATICFILES_DIR = BASE_DIR / "staticfiles"
 
-collect_ignore: List[str] = []
-
-if not env.ENABLE_FINDHELP:
-    collect_ignore.append("findhelp")
-
 
 @pytest.fixture
 def django_file_storage(settings):

deploy.py

@@ -261,6 +261,9 @@ def deploy(self) -> None:
             self.run_in_container(["python", "manage.py", "migrate"])
             self.run_in_container(["python", "manage.py", "initgroups"])
 
+            print("Loading geographic data...")
+            self.run_in_container(["python", "manage.py", "loadfindhelpdata"])
+
             print("Initiating Heroku release phase...")
             self.heroku.run("container:release", self.process_type, self.worker_process_type)
 

findhelp/admin.py

@@ -5,16 +5,7 @@
 from .admin_map import render_admin_map, admin_map_field, MapModelAdmin
 
 
-def register(model):
-    from project.settings import env
-
-    if env.ENABLE_FINDHELP:
-        return admin.register(model)
-
-    return lambda klass: klass
-
-
-@register(Zipcode)
+@admin.register(Zipcode)
 class ZipcodeAdmin(MapModelAdmin):
     list_display = ["zipcode"]
     search_fields = ["zipcode"]
@@ -24,7 +15,7 @@ class ZipcodeAdmin(MapModelAdmin):
     geometry = admin_map_field("geom", "Geometry")
 
 
-@register(Borough)
+@admin.register(Borough)
 class BoroughAdmin(MapModelAdmin):
     list_display = ["code", "name"]
     exclude = ["geom"]
@@ -33,7 +24,7 @@ class BoroughAdmin(MapModelAdmin):
     geometry = admin_map_field("geom", "Geometry")
 
 
-@register(Neighborhood)
+@admin.register(Neighborhood)
 class NeighborhoodAdmin(MapModelAdmin):
     list_display = ["name", "county"]
     search_fields = ["name"]
@@ -43,7 +34,7 @@ class NeighborhoodAdmin(MapModelAdmin):
     geometry = admin_map_field("geom", "Geometry")
 
 
-@register(CommunityDistrict)
+@admin.register(CommunityDistrict)
 class CommunityDistrictAdmin(MapModelAdmin):
     list_display = ["boro_cd", "name"]
     search_fields = ["name"]
@@ -53,7 +44,7 @@ class CommunityDistrictAdmin(MapModelAdmin):
     geometry = admin_map_field("geom", "Geometry")
 
 
-@register(TenantResource)
+@admin.register(TenantResource)
 class TenantResourceAdmin(MapModelAdmin):
     list_display = ["name", "org_type"]
     exclude = ["geocoded_point", "catchment_area"]

findhelp/management/commands/loadfindhelpdata.py

@@ -33,7 +33,7 @@ def get_or_construct(model, **kwargs):
 
 
 class Command(BaseCommand):
-    help = "Loads NYC geographic data into the database."
+    help = "Loads findhelp-related geographic data into the database."
 
     @transaction.atomic
     def handle(self, *args, **options):

findhelp/models.py

@@ -29,26 +29,6 @@ def union_geometries(geometries: Iterator[MultiPolygon]) -> Optional[MultiPolygo
     return to_multipolygon(total_area)
 
 
-class IgnoreFindhelpMigrationsRouter:
-    """
-    This is a database router that disables migrations related
-    to models in this app.  It can be used if a Django project
-    needs to optionally disable this app without necessarily
-    making its models un-introspectable.
-
-    We ultimately need this in order to define a consistent GraphQL
-    schema for the project without having to worry about whether
-    this particular app is disabled or not.
-    """
-
-    def allow_migrate(self, db, app_label, model_name=None, **hints):
-        if app_label == "findhelp":
-            return False
-        # Note that we are supposed to return None if we have
-        # no opinion on the matter, which is the case here.
-        return None
-
-
 class County(models.Model):
     class Meta:
         ordering = ["state", "name"]

findhelp/schema.py

@@ -51,17 +51,12 @@ class FindhelpInfo:
         longitude=graphene.Float(required=True),
         description=(
             "Find tenant resources that service the given location, ordered by their "
-            "proximity to the location. Note that if the tenant resource directory is "
-            "disabled on this endpoint, this will resolve to null."
+            "proximity to the location."
         ),
+        required=True,
     )
 
     def resolve_tenant_resources(self, info: ResolveInfo, latitude: float, longitude: float):
-        from project.settings import env
-
-        if not env.ENABLE_FINDHELP:
-            return None
-
         return TenantResource.objects.find_best_for(
             latitude=latitude,
             longitude=longitude,

findhelp/tests/conftest.py

@@ -2,11 +2,6 @@
 from types import SimpleNamespace
 import pytest
 
-from project import settings
-
-
-assert settings.env.ENABLE_FINDHELP, "findhelp tests should only run if ENABLE_FINDHELP is set!"
-
 
 class FakeGeocoder:
     def __init__(self):
@@ -32,10 +27,3 @@ def fake_geocoder(monkeypatch):
     fg = FakeGeocoder()
     monkeypatch.setattr(geocoding, "search", fg.search)
     return fg
-
-
-@pytest.fixture
-def simulate_findhelp_disabled(monkeypatch):
-    from project.settings import env
-
-    monkeypatch.setattr(env, "ENABLE_FINDHELP", False)

findhelp/tests/test_admin.py

@@ -1,12 +1,7 @@
-from findhelp.admin import register, TenantResourceAdmin
+from findhelp.admin import TenantResourceAdmin
 from findhelp.models import TenantResource
 
 
-def test_register_is_noop_if_findhelp_is_disabled(simulate_findhelp_disabled):
-    boop = "boop"
-    assert register("i am a fake model class")(boop) is boop
-
-
 class TestTenantResourceAdmin:
     def test_location_and_catchment_area_works(self):
         obj = TenantResource()

findhelp/tests/test_models.py

@@ -1,7 +1,6 @@
 from findhelp.models import (
     to_multipolygon,
     union_geometries,
-    IgnoreFindhelpMigrationsRouter,
     Zipcode,
     Borough,
     Neighborhood,
@@ -180,13 +179,3 @@ def test_iter_geometries_works(self, db):
         cd = create_cd()
         tr = create_tenant_resource(community_districts=[cd])
         assert union_geometries(tr.iter_geometries()) is not None
-
-
-class TestIgnoreFindhelpMigrationsRouter:
-    def test_it_returns_false_for_findhelp_models(self):
-        router = IgnoreFindhelpMigrationsRouter()
-        assert router.allow_migrate(None, app_label="findhelp") is False
-
-    def test_it_returns_none_for_non_findhelp_models(self):
-        router = IgnoreFindhelpMigrationsRouter()
-        assert router.allow_migrate(None, app_label="blarg") is None

findhelp/tests/test_schema.py

@@ -36,5 +36,5 @@ def test_it_works(self, db, fake_geocoder):
         assert results[1]["name"] == "Funky Help"
         assert int(results[1]["milesAway"]) == 40
 
-    def test_it_returns_none_if_findhelp_is_disabled(self, simulate_findhelp_disabled):
-        assert self.query(0.6, 0.5) is None
+    def test_it_returns_empty_list_if_nothing_is_found(self, db):
+        assert self.query(0.6, 0.5) == []

project/justfix_environment.py

@@ -21,7 +21,9 @@ class JustfixEnvironment(typed_environ.BaseEnvironment):
     #   https://github.com/kennethreitz/dj-database-url#url-schema
     #
     # Note that only Postgres/PostGIS are officially supported
-    # by this project.
+    # by this project. Note also that even if this URL uses the
+    # 'postgres:' protocol, the server will still expect the PostGIS
+    # extension to be installed on it at runtime.
     DATABASE_URL: str
 
     # The NYC-DB database URL. If empty, NYCDB integration will be
@@ -189,12 +191,6 @@ class JustfixEnvironment(typed_environ.BaseEnvironment):
     # 2FA will be disabled.
     TWOFACTOR_VERIFY_DURATION: int = 60 * 60 * 24
 
-    # Whether or not to enable the findhelp app, also known as
-    # the Tenant Assistance Directory. This requires that the
-    # default database be PostGIS, and that GeoDjango's requisite
-    # geospatial libraries are installed.
-    ENABLE_FINDHELP: bool = False
-
     # A Mapbox public access token for embedded maps and/or geocoding. If
     # not provided, mapbox integration will be disabled.
     MAPBOX_ACCESS_TOKEN: str = ""

project/settings.py

@@ -21,6 +21,7 @@
     parse_secure_proxy_ssl_header,
     parse_hostname_redirects,
     parse_comma_separated_list,
+    change_db_url_to_postgis,
     LazilyImportedFunction,
 )
 from .util import git
@@ -205,11 +206,8 @@
 
 DATABASE_ROUTERS: List[str] = []
 
-if not env.ENABLE_FINDHELP:
-    DATABASE_ROUTERS.append("findhelp.models.IgnoreFindhelpMigrationsRouter")
-
 DATABASES = {
-    "default": dj_database_url.parse(env.DATABASE_URL),
+    "default": dj_database_url.parse(change_db_url_to_postgis(env.DATABASE_URL)),
 }
 
 NYCDB_DATABASE = None

project/tests/test_deploy_snapshots/heroku_with_preboot.txt

@@ -8,6 +8,8 @@ Heroku preboot is enabled, proceeding with zero-downtime deploy.
 Running migrations...
 Running "docker run --rm -it -e DATABASE_URL registry.heroku.com/boop/web python manage.py migrate".
 Running "docker run --rm -it -e DATABASE_URL registry.heroku.com/boop/web python manage.py initgroups".
+Loading geographic data...
+Running "docker run --rm -it -e DATABASE_URL registry.heroku.com/boop/web python manage.py loadfindhelpdata".
 Initiating Heroku release phase...
 Running "heroku container:release web worker -r myapp".
 Deploy finished.

project/tests/test_deploy_snapshots/heroku_works.txt

@@ -9,6 +9,8 @@ Running "heroku maintenance:on -r myapp".
 Running migrations...
 Running "docker run --rm -it -e DATABASE_URL registry.heroku.com/boop/web python manage.py migrate".
 Running "docker run --rm -it -e DATABASE_URL registry.heroku.com/boop/web python manage.py initgroups".
+Loading geographic data...
+Running "docker run --rm -it -e DATABASE_URL registry.heroku.com/boop/web python manage.py loadfindhelpdata".
 Initiating Heroku release phase...
 Running "heroku container:release web worker -r myapp".
 Turning off maintenance mode.

project/tests/test_settings_util.py

@@ -5,6 +5,7 @@
     ensure_dependent_settings_are_nonempty,
     parse_comma_separated_list,
     LazilyImportedFunction,
+    change_db_url_to_postgis,
 )
 
 
@@ -48,3 +49,15 @@ def test_lazily_imported_function_works():
 )
 def test_parse_comma_separated_list_works(input, output):
     assert parse_comma_separated_list(input) == output
+
+
+class TestChangeDbUrlToPostgis:
+    def test_it_raises_error_on_unsupported_url_protocol(self):
+        with pytest.raises(ValueError, match="Expected URL to start with"):
+            change_db_url_to_postgis("sqlite://boop")
+
+    def test_it_does_nothing_to_postgis_urls(self):
+        assert change_db_url_to_postgis("postgis://boop") == "postgis://boop"
+
+    def test_it_changes_postgres_urls(self):
+        assert change_db_url_to_postgis("postgres://boop") == "postgis://boop"

project/util/settings_util.py

@@ -52,6 +52,25 @@ def ensure_dependent_settings_are_nonempty(setting: str, *dependent_settings: st
             raise ImproperlyConfigured(f"{setting} is non-empty, but {dependent_setting} is empty!")
 
 
+def change_db_url_to_postgis(url: str) -> str:
+    """
+    If the given URL is uses a `postgres:` protocol instead of a `postgis:` protocol,
+    modify it to use one, e.g.:
+
+        >>> change_db_url_to_postgis('postgres://blah')
+        'postgis://blah'
+    """
+
+    POSTGRES = "postgres://"
+    POSTGIS = "postgis://"
+
+    if url.startswith(POSTGRES):
+        url = POSTGIS + url[len(POSTGRES) :]
+    if not url.startswith(POSTGIS):
+        raise ValueError(f"Expected URL to start with '{POSTGRES}' or '{POSTGIS}': {repr(url)}")
+    return url
+
+
 class LazilyImportedFunction:
     """
     A class that can be used to import a function lazily,

schema.json

@@ -280,19 +280,23 @@
                 }
               ],
               "deprecationReason": null,
-              "description": "Find tenant resources that service the given location, ordered by their proximity to the location. Note that if the tenant resource directory is disabled on this endpoint, this will resolve to null.",
+              "description": "Find tenant resources that service the given location, ordered by their proximity to the location.",
               "isDeprecated": false,
               "name": "tenantResources",
               "type": {
-                "kind": "LIST",
+                "kind": "NON_NULL",
                 "name": null,
                 "ofType": {
-                  "kind": "NON_NULL",
+                  "kind": "LIST",
                   "name": null,
                   "ofType": {
-                    "kind": "OBJECT",
-                    "name": "TenantResourceType",
-                    "ofType": null
+                    "kind": "NON_NULL",
+                    "name": null,
+                    "ofType": {
+                      "kind": "OBJECT",
+                      "name": "TenantResourceType",
+                      "ofType": null
+                    }
                   }
                 }
               }

update.sh

@@ -19,3 +19,6 @@ yarn querybuilder
 echo "----- Migrating Database -----"
 python manage.py migrate --noinput
 python manage.py initgroups
+
+echo "----- Loading geographic data -----"
+python manage.py loadfindhelpdata