From a1977549060f732116b8ba02539e38694dbc5e88 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 15 Dec 2016 17:11:47 +0000 Subject: [PATCH 1/9] add new provider_details_history table * to be used for auditing changes to provider details * not hooked in yet * also made provider_details.active non-nullable. set all existing null provider_details.active to FALSE. none are set false on live so should hopefully be fairly uncontroversial * refactored NotificationHistory.from_notification and update from noti to share with provider_details_history --- app/dao/notifications_dao.py | 4 +- app/models.py | 35 +++++++++++++---- .../versions/0062_provider_details_history.py | 38 +++++++++++++++++++ 3 files changed, 67 insertions(+), 10 deletions(-) create mode 100644 migrations/versions/0062_provider_details_history.py diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 36901aecd..d62e62607 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -130,7 +130,7 @@ def dao_create_notification(notification): if not notification.status: notification.status = 'created' - notification_history = NotificationHistory.from_notification(notification) + notification_history = NotificationHistory.from_original(notification) db.session.add(notification) db.session.add(notification_history) @@ -190,7 +190,7 @@ def update_notification_status_by_reference(reference, status): def dao_update_notification(notification): notification.updated_at = datetime.utcnow() notification_history = NotificationHistory.query.get(notification.id) - notification_history.update_from_notification(notification) + notification_history.update_from_original(notification) db.session.add(notification) db.session.commit() diff --git a/app/models.py b/app/models.py index e7fbce1a0..a13eecc8b 100644 --- a/app/models.py +++ b/app/models.py @@ -35,6 +35,18 @@ def filter_null_value_fields(obj): ) +class HistoryModel: + @classmethod + def from_original(cls, original): + history = cls() + history.update_from_original(original) + return history + + def update_from_original(self, original): + for c in self.__table__.columns: + setattr(self, c.name, getattr(original, c.name)) + + class User(db.Model): __tablename__ = 'users' @@ -354,7 +366,18 @@ class ProviderDetails(db.Model): identifier = db.Column(db.String, nullable=False) priority = db.Column(db.Integer, nullable=False) notification_type = db.Column(notification_types, nullable=False) - active = db.Column(db.Boolean, default=False) + active = db.Column(db.Boolean, default=False, nullable=False) + + +class ProviderDetailsHistory(db.Model, HistoryModel): + __tablename__ = 'provider_details_history' + + id = db.Column(UUID(as_uuid=True), primary_key=True) + display_name = db.Column(db.String, nullable=False) + identifier = db.Column(db.String, nullable=False) + priority = db.Column(db.Integer, nullable=False) + notification_type = db.Column(notification_types, nullable=False) + active = db.Column(db.Boolean, nullable=False) JOB_STATUS_PENDING = 'pending' @@ -654,7 +677,7 @@ class Notification(db.Model): return serialized -class NotificationHistory(db.Model): +class NotificationHistory(db.Model, HistoryModel): __tablename__ = 'notification_history' id = db.Column(UUID(as_uuid=True), primary_key=True) @@ -680,14 +703,10 @@ class NotificationHistory(db.Model): client_reference = db.Column(db.String, nullable=True) @classmethod - def from_notification(cls, notification): - history = cls(**{c.name: getattr(notification, c.name) for c in cls.__table__.columns}) + def from_original(cls, notification): + history = super().from_original(notification) return history - def update_from_notification(self, notification): - for c in self.__table__.columns: - setattr(self, c.name, getattr(notification, c.name)) - INVITED_USER_STATUS_TYPES = ['pending', 'accepted', 'cancelled'] diff --git a/migrations/versions/0062_provider_details_history.py b/migrations/versions/0062_provider_details_history.py new file mode 100644 index 000000000..c4898970c --- /dev/null +++ b/migrations/versions/0062_provider_details_history.py @@ -0,0 +1,38 @@ +"""empty message + +Revision ID: 0062_provider_details_history +Revises: 0061_add_client_reference +Create Date: 2016-12-14 13:00:24.226990 + +""" + +# revision identifiers, used by Alembic. +revision = '0062_provider_details_history' +down_revision = '0061_add_client_reference' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +def upgrade(): + op.get_bind() + op.execute('UPDATE provider_details SET active = false WHERE active is null') + + op.alter_column('provider_details', 'active', existing_type=sa.BOOLEAN(), nullable=False) + + op.create_table('provider_details_history', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('display_name', sa.String(), nullable=False), + sa.Column('identifier', sa.String(), nullable=False), + sa.Column('priority', sa.Integer(), nullable=False), + sa.Column('notification_type', postgresql.ENUM('email', 'sms', 'letter', name='notification_type', create_type=False), nullable=False), + sa.Column('active', sa.Boolean(), nullable=False), + sa.PrimaryKeyConstraint('id') + ) + + +def downgrade(): + op.drop_table('provider_details_history') + + op.alter_column('provider_details_history', 'active', existing_type=sa.BOOLEAN(), nullable=True) + op.alter_column('provider_details', 'active', existing_type=sa.BOOLEAN(), nullable=True) From 0136e1e32d1b69fcaf933977c2f9ef7c2edd1d38 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 15 Dec 2016 17:30:05 +0000 Subject: [PATCH 2/9] fix invalid logging the first argument to ANY logger.____ function is ALWAYS cast to a string and used as a format argument for ALL remaining arguments using %s formatting. even `logger.exception`, which just logs as normal and then appends the stack trace. so we shouldn't be passing `e` into logger.exception - just `logger.exception('something went wrong!')` also de-duplicated a test --- app/celery/provider_tasks.py | 12 ++++-------- config.py | 2 +- tests/app/clients/test_firetext.py | 2 +- 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index ef98ec5e3..b0df268b3 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -43,15 +43,13 @@ def deliver_sms(self, notification_id): send_to_providers.send_sms_to_provider(notification) except Exception as e: try: - current_app.logger.error( + current_app.logger.exception( "RETRY: SMS notification {} failed".format(notification_id) ) - current_app.logger.exception(e) self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) except self.MaxRetriesExceededError: - current_app.logger.error( + current_app.logger.exception( "RETRY FAILED: task send_sms_to_provider failed for notification {}".format(notification_id), - e ) update_notification_status_by_id(notification_id, 'technical-failure') @@ -69,14 +67,12 @@ def deliver_email(self, notification_id): update_notification_status_by_id(notification_id, 'technical-failure') except Exception as e: try: - current_app.logger.error( + current_app.logger.exception( "RETRY: Email notification {} failed".format(notification_id) ) - current_app.logger.exception(e) self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) except self.MaxRetriesExceededError: current_app.logger.error( - "RETRY FAILED: task send_email_to_provider failed for notification {}".format(notification_id), - e + "RETRY FAILED: task send_email_to_provider failed for notification {}".format(notification_id) ) update_notification_status_by_id(notification_id, 'technical-failure') diff --git a/config.py b/config.py index 179d8fcf3..5c4105ffc 100644 --- a/config.py +++ b/config.py @@ -137,7 +137,7 @@ class Config(object): REDIS_ENABLED = False - SENDING_NOTIFICATIONS_TIMEOUT_PERIOD = 259200 + SENDING_NOTIFICATIONS_TIMEOUT_PERIOD = 259200 # 3 days SIMULATED_EMAIL_ADDRESSES = ('simulate-delivered@notifications.service.gov.uk', 'simulate-permanent-failure@notifications.service.gov.uk', diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py index 80737e23d..5685db48b 100644 --- a/tests/app/clients/test_firetext.py +++ b/tests/app/clients/test_firetext.py @@ -98,7 +98,7 @@ def test_send_sms_raises_if_firetext_rejects(mocker, mock_firetext_client): assert '"code": 1' in exc.value.text -def test_send_sms_raises_if_firetext_rejects(mocker, mock_firetext_client): +def test_send_sms_raises_if_firetext_rejects_with_unexpected_data(mocker, mock_firetext_client): to = content = reference = 'foo' response_dict = {"something": "gone bad"} From 9d1b1328afc765813c3c1ca295706a5cf37dee55 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 19 Dec 2016 16:49:56 +0000 Subject: [PATCH 3/9] add version to provider_details and _history set all existing rows to have a version of 1 (also copy across values to populate the new provider_details_history table in the upgrade script) in dao_update_provider_details bump the provider_details.version by 1 and then duplicate into the history table as a new row (done manually as opposed to the decorator used in template_history since this is only edited in this one place and the decorator is icky) --- app/dao/provider_details_dao.py | 5 ++++- app/models.py | 4 +++- .../versions/0062_provider_details_history.py | 16 +++++++++++++--- 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/app/dao/provider_details_dao.py b/app/dao/provider_details_dao.py index 275d74e5a..257858eee 100644 --- a/app/dao/provider_details_dao.py +++ b/app/dao/provider_details_dao.py @@ -1,6 +1,6 @@ from sqlalchemy import asc from app.dao.dao_utils import transactional -from app.models import ProviderDetails +from app.models import ProviderDetails, ProviderDetailsHistory from app import db @@ -20,4 +20,7 @@ def get_provider_details_by_notification_type(notification_type): @transactional def dao_update_provider_details(provider_details): + provider_details.version += 1 + history = ProviderDetailsHistory.from_original(provider_details) db.session.add(provider_details) + db.session.add(history) diff --git a/app/models.py b/app/models.py index a13eecc8b..35fac79f1 100644 --- a/app/models.py +++ b/app/models.py @@ -367,17 +367,19 @@ class ProviderDetails(db.Model): priority = db.Column(db.Integer, nullable=False) notification_type = db.Column(notification_types, nullable=False) active = db.Column(db.Boolean, default=False, nullable=False) + version = db.Column(db.Integer, default=1, nullable=False) class ProviderDetailsHistory(db.Model, HistoryModel): __tablename__ = 'provider_details_history' - id = db.Column(UUID(as_uuid=True), primary_key=True) + id = db.Column(UUID(as_uuid=True), primary_key=True, nullable=False) display_name = db.Column(db.String, nullable=False) identifier = db.Column(db.String, nullable=False) priority = db.Column(db.Integer, nullable=False) notification_type = db.Column(notification_types, nullable=False) active = db.Column(db.Boolean, nullable=False) + version = db.Column(db.Integer, primary_key=True, nullable=False) JOB_STATUS_PENDING = 'pending' diff --git a/migrations/versions/0062_provider_details_history.py b/migrations/versions/0062_provider_details_history.py index c4898970c..6ffc92927 100644 --- a/migrations/versions/0062_provider_details_history.py +++ b/migrations/versions/0062_provider_details_history.py @@ -16,9 +16,13 @@ from sqlalchemy.dialects import postgresql def upgrade(): op.get_bind() - op.execute('UPDATE provider_details SET active = false WHERE active is null') - op.alter_column('provider_details', 'active', existing_type=sa.BOOLEAN(), nullable=False) + op.execute('UPDATE provider_details SET active = false WHERE active is null') + op.alter_column('provider_details', 'active', nullable=False) + + op.add_column('provider_details', sa.Column('version', sa.Integer())) + op.execute('UPDATE provider_details SET version = 1') + op.alter_column('provider_details', 'version', nullable=False) op.create_table('provider_details_history', sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), @@ -27,12 +31,18 @@ def upgrade(): sa.Column('priority', sa.Integer(), nullable=False), sa.Column('notification_type', postgresql.ENUM('email', 'sms', 'letter', name='notification_type', create_type=False), nullable=False), sa.Column('active', sa.Boolean(), nullable=False), + sa.Column('version', sa.Integer(), nullable=False), sa.PrimaryKeyConstraint('id') ) + op.execute( + 'INSERT INTO provider_details_history' + + ' (id, display_name, identifier, priority, notification_type, active, version)' + + ' SELECT id, display_name, identifier, priority, notification_type, active, version FROM provider_details' + ) def downgrade(): op.drop_table('provider_details_history') - op.alter_column('provider_details_history', 'active', existing_type=sa.BOOLEAN(), nullable=True) op.alter_column('provider_details', 'active', existing_type=sa.BOOLEAN(), nullable=True) + op.drop_column('provider_details', 'version') From 8bb0261c79a9300277c237d24e02da75a7d78a03 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 19 Dec 2016 16:53:23 +0000 Subject: [PATCH 4/9] dont let user update prov details version, and refactor tests the provider details tests were previously very stateful - they would update a value, and then because provider_details is a "static" table that is not wiped by the notify_db_session fixture, the tests were forced to make a second update that reverted that change. if the test fails for whatever reason, the provider_details table ends up permanently modified, playing havoc on tests further down the line. this commit adds the fixture `restore_provider_details` to conftest. this fixture stores a copy of the contents of ProviderDetails and ProviderDetailsHistory tables outside of the session, runs the test, and then clears the table and puts those old values in this means that the tests have been cleaned up so that they do not do operations twice in reverse. they've also been cleaned up generally, including fixture optimisations and such --- app/provider_details/rest.py | 5 +- tests/app/conftest.py | 33 ++++ tests/app/provider_details/test_rest.py | 208 +++++++++--------------- tests/conftest.py | 2 +- 4 files changed, 114 insertions(+), 134 deletions(-) diff --git a/app/provider_details/rest.py b/app/provider_details/rest.py index b075653f5..4b7ae2a7a 100644 --- a/app/provider_details/rest.py +++ b/app/provider_details/rest.py @@ -37,9 +37,10 @@ def update_provider_details(provider_details_id): current_data.update(request.get_json()) update_dict = provider_details_schema.load(current_data).data - if "identifier" in request.get_json().keys(): + invalid_keys = {'identifier', 'version'} & set(key for key in request.get_json().keys()) + if invalid_keys: message = "Not permitted to be updated" - errors = {'identifier': [message]} + errors = {key: [message] for key in invalid_keys} raise InvalidRequest(errors, status_code=400) dao_update_provider_details(update_dict) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 674b99e31..8e570228b 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -1,6 +1,7 @@ import uuid from datetime import (datetime, date, timedelta) +from sqlalchemy.orm.session import make_transient import requests_mock import pytest from flask import current_app @@ -18,6 +19,8 @@ from app.models import ( Permission, ProviderStatistics, ProviderDetails, + ProviderDetailsHistory, + ProviderRates, NotificationStatistics, ServiceWhitelist, KEY_TYPE_NORMAL, KEY_TYPE_TEST, KEY_TYPE_TEAM, @@ -857,3 +860,33 @@ def sample_provider_rate(notify_db, notify_db_session, valid_from=None, rate=Non valid_from=valid_from if valid_from is not None else datetime.utcnow(), rate=rate if rate is not None else 1, ) + + +@pytest.fixture +def restore_provider_details(notify_db, notify_db_session): + """ + We view ProviderDetails as a static in notify_db_session, since we don't modify it... except we do, we updated + priority. This fixture is designed to be used in tests that will knowingly touch provider details, to restore them + to previous state. + + Note: This doesn't technically require notify_db_session (only notify_db), but kept as a requirement to encourage + good usage - if you're modifying ProviderDetails' state then it's good to clear down the rest of the DB too + """ + existing_provider_details = ProviderDetails.query.all() + existing_provider_details_history = ProviderDetailsHistory.query.all() + # make transient removes the objects from the session - since we'll want to delete them later + for epd in existing_provider_details: + make_transient(epd) + for epdh in existing_provider_details_history: + make_transient(epdh) + + yield + + # also delete these as they depend on provider_details + ProviderRates.query.delete() + ProviderDetails.query.delete() + ProviderDetailsHistory.query.delete() + notify_db.session.commit() + notify_db.session.add_all(existing_provider_details) + notify_db.session.add_all(existing_provider_details_history) + notify_db.session.commit() diff --git a/tests/app/provider_details/test_rest.py b/tests/app/provider_details/test_rest.py index b6a26a3e6..dc224e9b1 100644 --- a/tests/app/provider_details/test_rest.py +++ b/tests/app/provider_details/test_rest.py @@ -1,150 +1,96 @@ +import pytest from flask import json + +from app.models import ProviderDetails + from tests import create_authorization_header +def test_get_provider_details_in_type_and_identifier_order(client, notify_db): + response = client.get( + '/provider-details', + headers=[create_authorization_header()] + ) + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True))['provider_details'] + assert len(json_resp) == 4 -def test_get_provider_details_in_type_and_identifier_order(notify_db, notify_db_session, notify_api): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header() - response = client.get( - '/provider-details', - headers=[auth_header] - ) - assert response.status_code == 200 - json_resp = json.loads(response.get_data(as_text=True))['provider_details'] - assert len(json_resp) == 4 - - assert json_resp[0]['identifier'] == 'ses' - assert json_resp[1]['identifier'] == 'mmg' - assert json_resp[2]['identifier'] == 'firetext' - assert json_resp[3]['identifier'] == 'loadtesting' + assert json_resp[0]['identifier'] == 'ses' + assert json_resp[1]['identifier'] == 'mmg' + assert json_resp[2]['identifier'] == 'firetext' + assert json_resp[3]['identifier'] == 'loadtesting' -def test_get_provider_details_by_id(notify_db, notify_db_session, notify_api): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header() - response = client.get( - '/provider-details', - headers=[auth_header] - ) - json_resp = json.loads(response.get_data(as_text=True))['provider_details'] +def test_get_provider_details_by_id(client, notify_db): + response = client.get( + '/provider-details', + headers=[create_authorization_header()] + ) + json_resp = json.loads(response.get_data(as_text=True))['provider_details'] - provider_resp = client.get( - '/provider-details/{}'.format(json_resp[0]['id']), - headers=[auth_header] - ) + provider_resp = client.get( + '/provider-details/{}'.format(json_resp[0]['id']), + headers=[create_authorization_header()] + ) - provider = json.loads(provider_resp.get_data(as_text=True))['provider_details'] - assert provider['identifier'] == json_resp[0]['identifier'] + provider = json.loads(provider_resp.get_data(as_text=True))['provider_details'] + assert provider['identifier'] == json_resp[0]['identifier'] -def test_get_provider_details_contains_correct_fields(notify_db, notify_db_session, notify_api): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header() - response = client.get( - '/provider-details', - headers=[auth_header] - ) - json_resp = json.loads(response.get_data(as_text=True))['provider_details'] - allowed_keys = {"id", "display_name", "identifier", "priority", 'notification_type', "active"} - assert \ - allowed_keys == \ - set(json_resp[0].keys()) +def test_get_provider_details_contains_correct_fields(client, notify_db): + response = client.get( + '/provider-details', + headers=[create_authorization_header()] + ) + json_resp = json.loads(response.get_data(as_text=True))['provider_details'] + allowed_keys = {"id", "display_name", "identifier", "priority", 'notification_type', "active", "version"} + assert allowed_keys == set(json_resp[0].keys()) -def test_should_be_able_to_update_priority(notify_db, notify_db_session, notify_api): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header() - response = client.get( - '/provider-details', - headers=[auth_header] - ) - fetch_resp = json.loads(response.get_data(as_text=True))['provider_details'] +def test_should_be_able_to_update_priority(client, restore_provider_details): + provider = ProviderDetails.query.first() - provider_id = fetch_resp[2]['id'] - - update_resp = client.post( - '/provider-details/{}'.format(provider_id), - headers=[('Content-Type', 'application/json'), auth_header], - data=json.dumps({ - 'priority': 5 - }) - ) - assert update_resp.status_code == 200 - update_json = json.loads(update_resp.get_data(as_text=True))['provider_details'] - assert update_json['identifier'] == 'firetext' - assert update_json['priority'] == 5 - - update_resp = client.post( - '/provider-details/{}'.format(provider_id), - headers=[('Content-Type', 'application/json'), auth_header], - data=json.dumps({ - 'priority': 20 - }) - ) - assert update_resp.status_code == 200 + update_resp = client.post( + '/provider-details/{}'.format(provider.id), + headers=[('Content-Type', 'application/json'), create_authorization_header()], + data=json.dumps({ + 'priority': 5 + }) + ) + assert update_resp.status_code == 200 + update_json = json.loads(update_resp.get_data(as_text=True))['provider_details'] + assert update_json['identifier'] == provider.identifier + assert update_json['priority'] == 5 + assert provider.priority == 5 -def test_should_be_able_to_update_status(notify_db, notify_db_session, notify_api): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header() - response = client.get( - '/provider-details', - headers=[auth_header] - ) - fetch_resp = json.loads(response.get_data(as_text=True))['provider_details'] +def test_should_be_able_to_update_status(client, restore_provider_details): + provider = ProviderDetails.query.first() - firetext = next(x for x in fetch_resp if x['identifier'] == 'firetext') - - update_resp_1 = client.post( - '/provider-details/{}'.format(firetext['id']), - headers=[('Content-Type', 'application/json'), auth_header], - data=json.dumps({ - 'active': False - }) - ) - assert update_resp_1.status_code == 200 - update_resp_1 = json.loads(update_resp_1.get_data(as_text=True))['provider_details'] - assert update_resp_1['identifier'] == 'firetext' - assert not update_resp_1['active'] - - update_resp_2 = client.post( - '/provider-details/{}'.format(firetext['id']), - headers=[('Content-Type', 'application/json'), auth_header], - data=json.dumps({ - 'active': True - }) - ) - assert update_resp_2.status_code == 200 - update_resp_2 = json.loads(update_resp_2.get_data(as_text=True))['provider_details'] - assert update_resp_2['identifier'] == 'firetext' - assert update_resp_2['active'] + update_resp_1 = client.post( + '/provider-details/{}'.format(provider.id), + headers=[('Content-Type', 'application/json'), create_authorization_header()], + data=json.dumps({ + 'active': False + }) + ) + assert update_resp_1.status_code == 200 + update_resp_1 = json.loads(update_resp_1.get_data(as_text=True))['provider_details'] + assert update_resp_1['identifier'] == provider.identifier + assert not update_resp_1['active'] + assert not provider.active -def test_should_not_be_able_to_update_identifier(notify_db, notify_db_session, notify_api): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header() - response = client.get( - '/provider-details', - headers=[auth_header] - ) - fetch_resp = json.loads(response.get_data(as_text=True))['provider_details'] +@pytest.mark.parametrize('field,value', [('identifier', 'new'), ('version', 7)]) +def test_should_not_be_able_to_update_disallowed_fields(client, restore_provider_details, field, value): + provider = ProviderDetails.query.first() - provider_id = fetch_resp[2]['id'] - - update_resp = client.post( - '/provider-details/{}'.format(provider_id), - headers=[('Content-Type', 'application/json'), auth_header], - data=json.dumps({ - 'identifier': "new" - }) - ) - assert update_resp.status_code == 400 - update_resp = json.loads(update_resp.get_data(as_text=True)) - assert update_resp['message']['identifier'][0] == 'Not permitted to be updated' - assert update_resp['result'] == 'error' + update_resp = client.post( + '/provider-details/{}'.format(provider.id), + headers=[('Content-Type', 'application/json'), create_authorization_header()], + data=json.dumps({field: value}) + ) + assert update_resp.status_code == 400 + update_resp = json.loads(update_resp.get_data(as_text=True)) + print(update_resp) + assert update_resp['message'][field][0] == 'Not permitted to be updated' + assert update_resp['result'] == 'error' diff --git a/tests/conftest.py b/tests/conftest.py index 17beb8a01..13e705880 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -70,7 +70,7 @@ def notify_db_session(notify_db): notify_db.session.remove() for tbl in reversed(notify_db.metadata.sorted_tables): - if tbl.name not in ["provider_details", "key_types", "branding_type", "job_status"]: + if tbl.name not in ["provider_details", "key_types", "branding_type", "job_status", "provider_details_history"]: notify_db.engine.execute(tbl.delete()) notify_db.session.commit() From 9f88c310dcf54763a4fc85faf67c0e39488f5b3b Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 19 Dec 2016 17:10:15 +0000 Subject: [PATCH 5/9] remove useless test from provider_details_dao tests --- tests/app/dao/test_provider_details_dao.py | 34 +++++++--------------- 1 file changed, 10 insertions(+), 24 deletions(-) diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index 471632ebf..bcef14a88 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/tests/app/dao/test_provider_details_dao.py @@ -6,17 +6,17 @@ from app.dao.provider_details_dao import ( ) -def test_can_get_all_providers(notify_db, notify_db_session): +def test_can_get_all_providers(restore_provider_details): assert len(get_provider_details()) == 4 -def test_can_get_sms_providers(notify_db, notify_db_session): - assert len(get_provider_details_by_notification_type('sms')) == 3 - types = [provider.notification_type for provider in get_provider_details_by_notification_type('sms')] - assert all('sms' == notification_type for notification_type in types) +def test_can_get_sms_providers(restore_provider_details): + sms_providers = get_provider_details_by_notification_type('sms') + assert len(sms_providers) == 3 + assert all('sms' == prov.notification_type for prov in sms_providers) -def test_can_get_sms_providers_in_order(notify_db, notify_db_session): +def test_can_get_sms_providers_in_order(restore_provider_details): providers = get_provider_details_by_notification_type('sms') assert providers[0].identifier == "mmg" @@ -24,35 +24,21 @@ def test_can_get_sms_providers_in_order(notify_db, notify_db_session): assert providers[2].identifier == "loadtesting" -def test_can_get_email_providers_in_order(notify_db, notify_db_session): +def test_can_get_email_providers_in_order(restore_provider_details): providers = get_provider_details_by_notification_type('email') assert providers[0].identifier == "ses" -def test_can_get_email_providers(notify_db, notify_db_session): +def test_can_get_email_providers(restore_provider_details): assert len(get_provider_details_by_notification_type('email')) == 1 types = [provider.notification_type for provider in get_provider_details_by_notification_type('email')] assert all('email' == notification_type for notification_type in types) -def test_should_error_if_any_provider_in_database_not_in_code(notify_db, notify_db_session, notify_api): - providers = ProviderDetails.query.all() - - for provider in providers: - if provider.notification_type == 'sms': - assert clients.get_sms_client(provider.identifier) - if provider.notification_type == 'email': - assert clients.get_email_client(provider.identifier) - - -def test_should_not_error_if_any_provider_in_code_not_in_database(notify_db, notify_db_session, notify_api): +def test_should_not_error_if_any_provider_in_code_not_in_database(restore_provider_details): providers = ProviderDetails.query.all() ProviderDetails.query.filter_by(identifier='mmg').delete() - for provider in providers: - if provider.notification_type == 'sms': - assert clients.get_sms_client(provider.identifier) - if provider.notification_type == 'email': - assert clients.get_email_client(provider.identifier) + assert clients.get_sms_client('mmg') From 74cdcdcc928b7869625237cf2ceab4f3c0ce730e Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 19 Dec 2016 17:33:51 +0000 Subject: [PATCH 6/9] set prov details history's primary key to be id+version --- migrations/versions/0062_provider_details_history.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/migrations/versions/0062_provider_details_history.py b/migrations/versions/0062_provider_details_history.py index 6ffc92927..9b8145e58 100644 --- a/migrations/versions/0062_provider_details_history.py +++ b/migrations/versions/0062_provider_details_history.py @@ -32,7 +32,7 @@ def upgrade(): sa.Column('notification_type', postgresql.ENUM('email', 'sms', 'letter', name='notification_type', create_type=False), nullable=False), sa.Column('active', sa.Boolean(), nullable=False), sa.Column('version', sa.Integer(), nullable=False), - sa.PrimaryKeyConstraint('id') + sa.PrimaryKeyConstraint('id', 'version') ) op.execute( 'INSERT INTO provider_details_history' + From b39a027aeb360f71be616da559db06cf1d527836 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 19 Dec 2016 17:34:26 +0000 Subject: [PATCH 7/9] test to make sure provider details history is bumped --- tests/app/dao/test_provider_details_dao.py | 31 ++++++++++++++++++++-- tests/app/provider_details/test_rest.py | 1 + 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index bcef14a88..63db857f2 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/tests/app/dao/test_provider_details_dao.py @@ -1,8 +1,9 @@ -from app.models import ProviderDetails +from app.models import ProviderDetails, ProviderDetailsHistory from app import clients from app.dao.provider_details_dao import ( get_provider_details, - get_provider_details_by_notification_type + get_provider_details_by_notification_type, + dao_update_provider_details ) @@ -42,3 +43,29 @@ def test_should_not_error_if_any_provider_in_code_not_in_database(restore_provid ProviderDetails.query.filter_by(identifier='mmg').delete() assert clients.get_sms_client('mmg') + + +def test_update_adds_history(restore_provider_details): + ses = ProviderDetails.query.filter(ProviderDetails.identifier == 'ses').one() + ses_history = ProviderDetailsHistory.query.filter(ProviderDetailsHistory.id == ses.id).one() + + assert ses.version == 1 + assert ses_history.version == 1 + + ses.active = False + + dao_update_provider_details(ses) + + assert not ses.active + + ses_history = ProviderDetailsHistory.query.filter( + ProviderDetailsHistory.id == ses.id + ).order_by( + ProviderDetailsHistory.version + ).all() + + assert ses_history[0].active + assert ses_history[0].version == 1 + + assert not ses_history[1].active + assert ses_history[1].version == 2 diff --git a/tests/app/provider_details/test_rest.py b/tests/app/provider_details/test_rest.py index dc224e9b1..baafd431c 100644 --- a/tests/app/provider_details/test_rest.py +++ b/tests/app/provider_details/test_rest.py @@ -5,6 +5,7 @@ from app.models import ProviderDetails from tests import create_authorization_header + def test_get_provider_details_in_type_and_identifier_order(client, notify_db): response = client.get( '/provider-details', From f1899c6d53fbb15446348bbc52791162e8a92201 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 19 Dec 2016 17:45:46 +0000 Subject: [PATCH 8/9] add updated_at to provider_details it's set to utcnow from dao_update_provider_details --- app/dao/provider_details_dao.py | 3 +++ app/models.py | 2 ++ app/provider_details/rest.py | 2 +- migrations/versions/0062_provider_details_history.py | 10 ++++++++-- tests/app/dao/test_provider_details_dao.py | 9 +++++++++ tests/app/provider_details/test_rest.py | 10 ++++++++-- 6 files changed, 31 insertions(+), 5 deletions(-) diff --git a/app/dao/provider_details_dao.py b/app/dao/provider_details_dao.py index 257858eee..9d559ee0b 100644 --- a/app/dao/provider_details_dao.py +++ b/app/dao/provider_details_dao.py @@ -1,3 +1,5 @@ +from datetime import datetime + from sqlalchemy import asc from app.dao.dao_utils import transactional from app.models import ProviderDetails, ProviderDetailsHistory @@ -21,6 +23,7 @@ def get_provider_details_by_notification_type(notification_type): @transactional def dao_update_provider_details(provider_details): provider_details.version += 1 + provider_details.updated_at = datetime.utcnow() history = ProviderDetailsHistory.from_original(provider_details) db.session.add(provider_details) db.session.add(history) diff --git a/app/models.py b/app/models.py index 35fac79f1..8e266e46f 100644 --- a/app/models.py +++ b/app/models.py @@ -368,6 +368,7 @@ class ProviderDetails(db.Model): notification_type = db.Column(notification_types, nullable=False) active = db.Column(db.Boolean, default=False, nullable=False) version = db.Column(db.Integer, default=1, nullable=False) + updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) class ProviderDetailsHistory(db.Model, HistoryModel): @@ -380,6 +381,7 @@ class ProviderDetailsHistory(db.Model, HistoryModel): notification_type = db.Column(notification_types, nullable=False) active = db.Column(db.Boolean, nullable=False) version = db.Column(db.Integer, primary_key=True, nullable=False) + updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) JOB_STATUS_PENDING = 'pending' diff --git a/app/provider_details/rest.py b/app/provider_details/rest.py index 4b7ae2a7a..35b84c077 100644 --- a/app/provider_details/rest.py +++ b/app/provider_details/rest.py @@ -37,7 +37,7 @@ def update_provider_details(provider_details_id): current_data.update(request.get_json()) update_dict = provider_details_schema.load(current_data).data - invalid_keys = {'identifier', 'version'} & set(key for key in request.get_json().keys()) + invalid_keys = {'identifier', 'version', 'updated_at'} & set(key for key in request.get_json().keys()) if invalid_keys: message = "Not permitted to be updated" errors = {key: [message] for key in invalid_keys} diff --git a/migrations/versions/0062_provider_details_history.py b/migrations/versions/0062_provider_details_history.py index 9b8145e58..823f483af 100644 --- a/migrations/versions/0062_provider_details_history.py +++ b/migrations/versions/0062_provider_details_history.py @@ -1,4 +1,7 @@ -"""empty message +""" +* add version and updated_at to provider_details +* set active to not nullable (any existing null is set to false) +* create provider_details_history table, mimicking provider_details Revision ID: 0062_provider_details_history Revises: 0061_add_client_reference @@ -16,11 +19,12 @@ from sqlalchemy.dialects import postgresql def upgrade(): op.get_bind() + op.add_column('provider_details', sa.Column('updated_at', sa.DateTime())) op.execute('UPDATE provider_details SET active = false WHERE active is null') op.alter_column('provider_details', 'active', nullable=False) - op.add_column('provider_details', sa.Column('version', sa.Integer())) + op.add_column('provider_details', sa.Column('version', sa.Integer(), nullable=True)) op.execute('UPDATE provider_details SET version = 1') op.alter_column('provider_details', 'version', nullable=False) @@ -32,6 +36,7 @@ def upgrade(): sa.Column('notification_type', postgresql.ENUM('email', 'sms', 'letter', name='notification_type', create_type=False), nullable=False), sa.Column('active', sa.Boolean(), nullable=False), sa.Column('version', sa.Integer(), nullable=False), + sa.Column('updated_at', sa.DateTime(), nullable=True), sa.PrimaryKeyConstraint('id', 'version') ) op.execute( @@ -46,3 +51,4 @@ def downgrade(): op.alter_column('provider_details', 'active', existing_type=sa.BOOLEAN(), nullable=True) op.drop_column('provider_details', 'version') + op.drop_column('provider_details', 'updated_at') diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index 63db857f2..1bb2d8c09 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/tests/app/dao/test_provider_details_dao.py @@ -1,3 +1,7 @@ +from datetime import datetime + +from freezegun import freeze_time + from app.models import ProviderDetails, ProviderDetailsHistory from app import clients from app.dao.provider_details_dao import ( @@ -45,18 +49,21 @@ def test_should_not_error_if_any_provider_in_code_not_in_database(restore_provid assert clients.get_sms_client('mmg') +@freeze_time('2000-01-01T00:00:00') def test_update_adds_history(restore_provider_details): ses = ProviderDetails.query.filter(ProviderDetails.identifier == 'ses').one() ses_history = ProviderDetailsHistory.query.filter(ProviderDetailsHistory.id == ses.id).one() assert ses.version == 1 assert ses_history.version == 1 + assert ses.updated_at is None ses.active = False dao_update_provider_details(ses) assert not ses.active + assert ses.updated_at == datetime(2000, 1, 1, 0, 0, 0) ses_history = ProviderDetailsHistory.query.filter( ProviderDetailsHistory.id == ses.id @@ -66,6 +73,8 @@ def test_update_adds_history(restore_provider_details): assert ses_history[0].active assert ses_history[0].version == 1 + assert ses_history[0].updated_at is None assert not ses_history[1].active assert ses_history[1].version == 2 + assert ses_history[1].updated_at == datetime(2000, 1, 1, 0, 0, 0) diff --git a/tests/app/provider_details/test_rest.py b/tests/app/provider_details/test_rest.py index baafd431c..b0a739296 100644 --- a/tests/app/provider_details/test_rest.py +++ b/tests/app/provider_details/test_rest.py @@ -43,7 +43,9 @@ def test_get_provider_details_contains_correct_fields(client, notify_db): headers=[create_authorization_header()] ) json_resp = json.loads(response.get_data(as_text=True))['provider_details'] - allowed_keys = {"id", "display_name", "identifier", "priority", 'notification_type', "active", "version"} + allowed_keys = { + "id", "display_name", "identifier", "priority", 'notification_type', "active", "version", "updated_at" + } assert allowed_keys == set(json_resp[0].keys()) @@ -81,7 +83,11 @@ def test_should_be_able_to_update_status(client, restore_provider_details): assert not provider.active -@pytest.mark.parametrize('field,value', [('identifier', 'new'), ('version', 7)]) +@pytest.mark.parametrize('field,value', [ + ('identifier', 'new'), + ('version', 7), + ('updated_at', None) +]) def test_should_not_be_able_to_update_disallowed_fields(client, restore_provider_details, field, value): provider = ProviderDetails.query.first() From 31b2c3f4029c82e8630f3ed315e658072161867a Mon Sep 17 00:00:00 2001 From: bandesz Date: Tue, 20 Dec 2016 11:03:25 +0000 Subject: [PATCH 9/9] Fix apt proxy in Docker --- docker/Dockerfile-build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/Dockerfile-build b/docker/Dockerfile-build index 872c8d323..b8d145cb5 100644 --- a/docker/Dockerfile-build +++ b/docker/Dockerfile-build @@ -9,7 +9,7 @@ ENV PYTHONUNBUFFERED=1 \ RUN \ echo "Install base packages" \ - && ([ -z "$HTTP_PROXY" ] || echo "Acquire::http::Proxy \"${HTTP_PROXY}\";\n" > /etc/apt/apt.conf.d/99HttpProxy) \ + && ([ -z "$HTTP_PROXY" ] || echo "Acquire::http::Proxy \"${HTTP_PROXY}\";" > /etc/apt/apt.conf.d/99HttpProxy) \ && apt-get update \ && apt-get install -y --no-install-recommends \ make \