Adapt the app for Councilmatic 2.5 #34

jeancochrane · 2019-07-23T18:22:08Z

Overview

Make sure the app is compatible with django-councilmatic v2.5. The major changes in this update include:

Support Django 2.0+
Refactor queries in send_notifications management command to use the ORM and the new OCD model structure
Write some basic tests for the app

Notes

This PR supercedes Adapt the app for Councilmatic 1.0 #33. The diff between the two branches was large, and conversation on that PR had gotten quite long, so I decided to open a fresh one.

Testing instructions

Make a virtualenv for the project and install it for testing with pip install -e .[tests]
Run pytest and confirm all tests pass

…ncilmatic

notifications/management/commands/send_notifications.py

hancush · 2019-07-23T20:24:04Z

hm, @jeancochrane, i'm getting test failures, even after installing django-councilmatic as instructed:

(django-councilmatic-notifications) call-me-hank:django-councilmatic-notifications hannah$ pytest
================================================ test session starts ================================================
platform darwin -- Python 3.7.3, pytest-5.0.1, py-1.8.0, pluggy-0.12.0
Django settings: tests.test_config (from ini file)
rootdir: /Users/hannah/projects/django-councilmatic-notifications, inifile: setup.cfg
plugins: django-3.5.1, mock-1.10.4
collected 13 items

tests/test_management_commands.py .F.....F...                                                                 [ 84%]
tests/test_views.py ..                                                                                        [100%]

===================================================== FAILURES ======================================================
__________________________________ test_find_bill_action_updates_skips_null_dates ___________________________________

self = <django.db.backends.utils.CursorWrapper object at 0x10f9b6320>
sql = 'SELECT "opencivicdata_billaction"."id", "opencivicdata_billaction"."bill_id", "opencivicdata_billaction"."organizatio...D ("opencivicdata_billaction"."date")::timestamp with time zone >= %s) ORDER BY "opencivicdata_billaction"."order" ASC'
params = ('ocd-bill/19cf632a-8323-4c8f-afb3-9d5294b2e7cd', datetime.datetime(2019, 7, 23, 15, 7, 45, 542694, tzinfo=<DstTzInfo 'America/Chicago' CDT-1 day, 19:00:00 DST>))
ignored_wrapper_args = (False, {'connection': <django.contrib.gis.db.backends.postgis.base.DatabaseWrapper object at 0x10bf66e10>, 'cursor': <django.db.backends.utils.CursorWrapper object at 0x10f9b6320>})

    def _execute(self, sql, params, *ignored_wrapper_args):
        self.db.validate_no_broken_transaction()
        with self.db.wrap_database_errors:
            if params is None:
                return self.cursor.execute(sql)
            else:
>               return self.cursor.execute(sql, params)
E               psycopg2.errors.InvalidDatetimeFormat: invalid input syntax for type timestamp with time zone: ""

../../.virtualenvs/django-councilmatic-notifications/lib/python3.7/site-packages/django/db/backends/utils.py:85: InvalidDatetimeFormat

The above exception was the direct cause of the following exception:

new_bill = <Bill: test bill>
new_bill_actions = (<BillAction: test bill action on >, <BillAction: test bill action on >)

    @pytest.mark.django_db
    def test_find_bill_action_updates_skips_null_dates(new_bill, new_bill_actions):
        for bill_action in new_bill_actions:
            # Null dates are saved as empty strings in the OCD data model
            bill_action.date = ''
            bill_action.save()
        command = Command()
>       bill_action_updates = command.find_bill_action_updates([new_bill.id], minutes=15)

tests/test_management_commands.py:33:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
notifications/management/commands/send_notifications.py:209: in find_bill_action_updates
    for action in new_actions:
../../.virtualenvs/django-councilmatic-notifications/lib/python3.7/site-packages/django/db/models/query.py:268: in __iter__
    self._fetch_all()
../../.virtualenvs/django-councilmatic-notifications/lib/python3.7/site-packages/django/db/models/query.py:1186: in _fetch_all
    self._result_cache = list(self._iterable_class(self))
../../.virtualenvs/django-councilmatic-notifications/lib/python3.7/site-packages/django/db/models/query.py:54: in __iter__
    results = compiler.execute_sql(chunked_fetch=self.chunked_fetch, chunk_size=self.chunk_size)
../../.virtualenvs/django-councilmatic-notifications/lib/python3.7/site-packages/django/db/models/sql/compiler.py:1065: in execute_sql
    cursor.execute(sql, params)
../../.virtualenvs/django-councilmatic-notifications/lib/python3.7/site-packages/django/db/backends/utils.py:68: in execute
    return self._execute_with_wrappers(sql, params, many=False, executor=self._execute)
../../.virtualenvs/django-councilmatic-notifications/lib/python3.7/site-packages/django/db/backends/utils.py:77: in _execute_with_wrappers
    return executor(sql, params, many, context)
../../.virtualenvs/django-councilmatic-notifications/lib/python3.7/site-packages/django/db/backends/utils.py:85: in _execute
    return self.cursor.execute(sql, params)
../../.virtualenvs/django-councilmatic-notifications/lib/python3.7/site-packages/django/db/utils.py:89: in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <django.db.backends.utils.CursorWrapper object at 0x10f9b6320>
sql = 'SELECT "opencivicdata_billaction"."id", "opencivicdata_billaction"."bill_id", "opencivicdata_billaction"."organizatio...D ("opencivicdata_billaction"."date")::timestamp with time zone >= %s) ORDER BY "opencivicdata_billaction"."order" ASC'
params = ('ocd-bill/19cf632a-8323-4c8f-afb3-9d5294b2e7cd', datetime.datetime(2019, 7, 23, 15, 7, 45, 542694, tzinfo=<DstTzInfo 'America/Chicago' CDT-1 day, 19:00:00 DST>))
ignored_wrapper_args = (False, {'connection': <django.contrib.gis.db.backends.postgis.base.DatabaseWrapper object at 0x10bf66e10>, 'cursor': <django.db.backends.utils.CursorWrapper object at 0x10f9b6320>})

    def _execute(self, sql, params, *ignored_wrapper_args):
        self.db.validate_no_broken_transaction()
        with self.db.wrap_database_errors:
            if params is None:
                return self.cursor.execute(sql)
            else:
>               return self.cursor.execute(sql, params)
E               django.db.utils.DataError: invalid input syntax for type timestamp with time zone: ""

../../.virtualenvs/django-councilmatic-notifications/lib/python3.7/site-packages/django/db/backends/utils.py:85: DataError
_______________________________________ test_find_new_events_skips_null_dates _______________________________________

self = <django.db.backends.utils.CursorWrapper object at 0x10f839d68>
sql = 'SELECT "opencivicdata_event"."created_at", "opencivicdata_event"."updated_at", "opencivicdata_event"."extras", "openc...created_at" >= %s AND ("opencivicdata_event"."start_date")::timestamp with time zone >= %s) ORDER BY "start_time" DESC'
params = (datetime.datetime(2019, 7, 23, 15, 7, 45, 892043, tzinfo=<DstTzInfo 'America/Chicago' CDT-1 day, 19:00:00 DST>), datetime.datetime(2019, 7, 23, 15, 22, 45, 892065, tzinfo=<DstTzInfo 'America/Chicago' CDT-1 day, 19:00:00 DST>))
ignored_wrapper_args = (False, {'connection': <django.contrib.gis.db.backends.postgis.base.DatabaseWrapper object at 0x10bf66e10>, 'cursor': <django.db.backends.utils.CursorWrapper object at 0x10f839d68>})

    def _execute(self, sql, params, *ignored_wrapper_args):
        self.db.validate_no_broken_transaction()
        with self.db.wrap_database_errors:
            if params is None:
                return self.cursor.execute(sql)
            else:
>               return self.cursor.execute(sql, params)
E               psycopg2.errors.InvalidDatetimeFormat: invalid input syntax for type timestamp with time zone: ""

../../.virtualenvs/django-councilmatic-notifications/lib/python3.7/site-packages/django/db/backends/utils.py:85: InvalidDatetimeFormat

The above exception was the direct cause of the following exception:

new_events = (<Event: test event 1 ()>, <Event: test event 2 ()>)

    @pytest.mark.django_db
    def test_find_new_events_skips_null_dates(new_events):
        for event in new_events:
            # Null dates are saved as empty strings in the OCD data model
            event.start_date = ''
            event.save()
        command = Command()
>       found_events = command.find_new_events()

tests/test_management_commands.py:121:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
notifications/management/commands/send_notifications.py:385: in find_new_events
    for event in new_events_q:
../../.virtualenvs/django-councilmatic-notifications/lib/python3.7/site-packages/django/db/models/query.py:268: in __iter__
    self._fetch_all()
../../.virtualenvs/django-councilmatic-notifications/lib/python3.7/site-packages/django/db/models/query.py:1186: in _fetch_all
    self._result_cache = list(self._iterable_class(self))
../../.virtualenvs/django-councilmatic-notifications/lib/python3.7/site-packages/django/db/models/query.py:54: in __iter__
    results = compiler.execute_sql(chunked_fetch=self.chunked_fetch, chunk_size=self.chunk_size)
../../.virtualenvs/django-councilmatic-notifications/lib/python3.7/site-packages/django/db/models/sql/compiler.py:1065: in execute_sql
    cursor.execute(sql, params)
../../.virtualenvs/django-councilmatic-notifications/lib/python3.7/site-packages/django/db/backends/utils.py:68: in execute
    return self._execute_with_wrappers(sql, params, many=False, executor=self._execute)
../../.virtualenvs/django-councilmatic-notifications/lib/python3.7/site-packages/django/db/backends/utils.py:77: in _execute_with_wrappers
    return executor(sql, params, many, context)
../../.virtualenvs/django-councilmatic-notifications/lib/python3.7/site-packages/django/db/backends/utils.py:85: in _execute
    return self.cursor.execute(sql, params)
../../.virtualenvs/django-councilmatic-notifications/lib/python3.7/site-packages/django/db/utils.py:89: in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <django.db.backends.utils.CursorWrapper object at 0x10f839d68>
sql = 'SELECT "opencivicdata_event"."created_at", "opencivicdata_event"."updated_at", "opencivicdata_event"."extras", "openc...created_at" >= %s AND ("opencivicdata_event"."start_date")::timestamp with time zone >= %s) ORDER BY "start_time" DESC'
params = (datetime.datetime(2019, 7, 23, 15, 7, 45, 892043, tzinfo=<DstTzInfo 'America/Chicago' CDT-1 day, 19:00:00 DST>), datetime.datetime(2019, 7, 23, 15, 22, 45, 892065, tzinfo=<DstTzInfo 'America/Chicago' CDT-1 day, 19:00:00 DST>))
ignored_wrapper_args = (False, {'connection': <django.contrib.gis.db.backends.postgis.base.DatabaseWrapper object at 0x10bf66e10>, 'cursor': <django.db.backends.utils.CursorWrapper object at 0x10f839d68>})

    def _execute(self, sql, params, *ignored_wrapper_args):
        self.db.validate_no_broken_transaction()
        with self.db.wrap_database_errors:
            if params is None:
                return self.cursor.execute(sql)
            else:
>               return self.cursor.execute(sql, params)
E               django.db.utils.DataError: invalid input syntax for type timestamp with time zone: ""

../../.virtualenvs/django-councilmatic-notifications/lib/python3.7/site-packages/django/db/backends/utils.py:85: DataError
======================================= 2 failed, 11 passed in 20.69 seconds ========================================

jeancochrane · 2019-07-23T20:56:47Z

@hancush Did you switch to the new branch (feature/jfc/councilmatic-2.5)? I removed the test test_find_bill_action_updates_skips_null_dates in this branch but it seems to be showing up in your traceback.

hancush · 2019-07-23T21:03:13Z

hhhhhmmmmmmmmmmmmm @ myself. thanks, @jeancochrane, you're right. sorry about that.

jeancochrane · 2019-07-24T17:33:25Z

@hancush Since django-councilmatic now has a v2.5.0 release, I updated this PR to pin django-councilmatic>=2.5 and set the version of this app to 1.0.0. You should now be able to set up this env simply by running pip install -e .[tests].

hancush

this is great, @jeancochrane. thank you so much for this thoughtful refactor. left a few commends/questions inline.

notifications/management/commands/send_notifications.py

hancush · 2019-07-24T18:20:57Z

notifications/models.py

-from django.contrib.postgres.fields import JSONField
+try:
+    HAYSTACK_URL = settings.HAYSTACK_CONNECTIONS['default']['URL']
+except KeyError:


this is totally up to your judgment: do you think we should log a warning that solr is not configured if the app starts without this setting, either in addition to or instead of logging when the management command is sent?

I like that idea a lot, but do you have any sense of how we'd hook into the app start event to emit the warning? Not sure when that happens and I can't find anything from a cursory search.

i think that this will get evaluated when you start the app, no extra steps required. (added a print statement to the except block to test.)

(chi-councilmatic) call-me-hank:chi-councilmatic hannah$ python manage.py runserver no haystack url

that said, as i was testing that assumption, i learned haystack itself will raise a fatal exception if you don't define the default connection, so maybe we don't need to do anything ourselves.

raise ImproperlyConfigured("The default alias '%s' must be included in the HAYSTACK_CONNECTIONS setting." % DEFAULT_ALIAS) django.core.exceptions.ImproperlyConfigured: The default alias 'default' must be included in the HAYSTACK_CONNECTIONS setting.

what do you think?

Nice! I think the built-in error is definitely more informative than ours anyway. The only open question to me is whether or not there's a use-case for running this app without search support. If so, it might make more sense to handle the error gently with a warning rather than raise a fatal error IMO. But if we imagine that wiring up Haystack is a necessary prerequisite for the app, then I think we can safely get rid of our error and rely on Haystack to complain.

haystack is a pre-req of django-councilmatic, so if we can't envision running notifications without django-councilmatic, then i think haystack is a necessary pre-req here. does that logic check out?

Makes sense to me 👍

i think we we landed on the app not being able to run without search, i.e., we can rely on haystack to raise the configuration errors.

is there any reason to keep the error handling in:

this check?

BillSearchSubscription.get_updates?

send_notifications?

(apologies if this is obvious, i may need some more caffeine.)

To me, these extra checks seem like reasonable defensive programming in the face of an uncertain integration. We expect Haystack to raise a configuration error if its configuration dictionary is not properly defined, but we also don't control Haystack or its startup logic, so we can't have perfect confidence that it will error out when we want it to. Plus, when we access HAYSTACK_URL, it's in a nested dictionary on the settings object, so if any part of settings.HAYSTACK_CONNECTIONS['default']['URL'] is not defined the user will get a super unhlpeful error. Does this judgment seem reasonable to you, or do you think it's overly confusing to be extra cautious around HAYSTACK_URL?

while it's true we don't control haystack, we do control the version of haystack we want to run, so we have a way of ensuring consistency if future releases of haystack behave differently than the one we're using now, which does raise the configuration error as we expect.

that said, i do find more useful exceptions a compelling reason to preserve the custom exception handling code. this isn't a change i require.

notifications/models.py

notifications/management/commands/send_notifications.py

jeancochrane

Thanks for your comments @hancush! I followed up with a couple questions for you. For the ones that have immediate action steps, I'll go ahead and make the changes and then re-request your review.

jeancochrane · 2019-07-24T19:16:00Z

notifications/models.py

-from django.contrib.postgres.fields import JSONField
+try:
+    HAYSTACK_URL = settings.HAYSTACK_CONNECTIONS['default']['URL']
+except KeyError:


I like that idea a lot, but do you have any sense of how we'd hook into the app start event to emit the warning? Not sure when that happens and I can't find anything from a cursory search.

notifications/models.py

notifications/management/commands/send_notifications.py

… be local to handle()

jeancochrane

@hancush I think I responded to all of your comments! Look like there's anything I missed?

hancush

although it was bigger than anticipated, this is a better refactor than i could have imagined! great work, @jeancochrane, and thank you. i left a couple of comments inline, but i'm happy to approve.

hancush · 2019-07-31T14:23:59Z

notifications/management/commands/send_notifications.py

-                cursor.execute(new_bills, [ocd_ids])
-
-                bills = []
+    def _is_empty(self, elem):


does this lil guy need to stick around now that you've defined it locally?

Oops, great catch! Removed in f74e508.

hancush · 2019-07-31T15:23:38Z

notifications/models.py

-from django.contrib.postgres.fields import JSONField
+try:
+    HAYSTACK_URL = settings.HAYSTACK_CONNECTIONS['default']['URL']
+except KeyError:


i think we we landed on the app not being able to run without search, i.e., we can rely on haystack to raise the configuration errors.

is there any reason to keep the error handling in:

this check?

BillSearchSubscription.get_updates?

send_notifications?

(apologies if this is obvious, i may need some more caffeine.)

Jean Cochrane (Lead developer, DataMade) added 20 commits June 27, 2019 17:03

Support Django 2

8c0d5e5

Configure tests for the app

03050e7

Stub out tests and get pytest-django up and running

dba4aea

Get first test passing

aa2b8a3

Create fixtures and test for person subscription

fa92f55

Add failing tests for send_notifications mgmt command

3cd3ba6

Adjust find_bill_actions_updates for new OCD models

a977454

Fix up some more send_notifications tests

9706068

Refactor send_notifications queries for new and updated events

3af36fb

Refactor send_notifications tests to use common fixtures

7bb46da

Remove unused metro fixtures from conftest

da13c54

Remove debug_toolbar from test_config

d9e2655

Refactor NULL-casting logic from send_notifications out to django-cou…

ed41cad

…ncilmatic

Add tests confirming that null dates get filtered out of queries

cb36d7b

Remove unused imports from send_noficiations.py

534b546

Remove unecessary distinct() filters from send_notifications queries

62fc2ca

WIP refactor query logic onto models

785345d

WIP update tests for model refactor

9a1d07f

Fix up tests for Person and CommitteeAction subscriptions

2241529

Write integration tests and fix tests for CommitteeEvents

ffce790

jeancochrane changed the base branch from master to 2.5 July 23, 2019 18:24

Clean up diff ahead of review

a0f074f

jeancochrane commented Jul 23, 2019

View reviewed changes

notifications/management/commands/send_notifications.py Show resolved Hide resolved

jeancochrane requested a review from hancush July 23, 2019 18:43

jeancochrane mentioned this pull request Jul 24, 2019

Adapt the app for Councilmatic 1.0 #33

Closed

Bump version to 1.0.0 and pin to django-councilmatic>=2.5

5470e8e

hancush reviewed Jul 24, 2019

View reviewed changes

jeancochrane commented Jul 24, 2019

View reviewed changes

Document BillActionSubscription.get_updates() and move not_empty() to…

9a3c3f0

… be local to handle()

jeancochrane commented Jul 30, 2019

View reviewed changes

hancush approved these changes Jul 31, 2019

View reviewed changes

Remove deprecated is_empty() method from send_notifications.py

f74e508

jeancochrane merged commit 7a28a3b into 2.5 Jul 31, 2019

jeancochrane deleted the feature/jfc/councilmatic-2.5 branch July 31, 2019 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt the app for Councilmatic 2.5 #34

Adapt the app for Councilmatic 2.5 #34

jeancochrane commented Jul 23, 2019 •

edited

Loading

hancush commented Jul 23, 2019

jeancochrane commented Jul 23, 2019

hancush commented Jul 23, 2019

jeancochrane commented Jul 24, 2019

hancush left a comment

hancush Jul 24, 2019

jeancochrane Jul 24, 2019

hancush Jul 24, 2019

jeancochrane Jul 24, 2019

hancush Jul 24, 2019

jeancochrane Jul 24, 2019

hancush Jul 31, 2019

jeancochrane Jul 31, 2019

hancush Jul 31, 2019

jeancochrane Jul 31, 2019

jeancochrane left a comment

jeancochrane Jul 24, 2019

jeancochrane left a comment

hancush left a comment

hancush Jul 31, 2019

jeancochrane Jul 31, 2019

hancush Jul 31, 2019

Adapt the app for Councilmatic 2.5 #34

Adapt the app for Councilmatic 2.5 #34

Conversation

jeancochrane commented Jul 23, 2019 • edited Loading

Overview

Notes

Testing instructions

hancush commented Jul 23, 2019

jeancochrane commented Jul 23, 2019

hancush commented Jul 23, 2019

jeancochrane commented Jul 24, 2019

hancush left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeancochrane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeancochrane left a comment

Choose a reason for hiding this comment

hancush left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeancochrane commented Jul 23, 2019 •

edited

Loading