From 721202b44fd837f9017a72b5eeb018611da7b327 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 28 Nov 2017 10:35:46 +0000 Subject: [PATCH 1/9] use more click functionality to reduce our own codes' complexity --- app/commands.py | 29 ++++++++++------------------- 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/app/commands.py b/app/commands.py index 1c7918315..4a8ae4f26 100644 --- a/app/commands.py +++ b/app/commands.py @@ -49,25 +49,16 @@ class notify_command: @notify_command() -@click.option('-p', '--provider_name', required=True, help='Provider name') -@click.option('-c', '--cost', required=True, help='Cost (pence) per message including decimals') +@click.option('-p', '--provider_name', required=True, help='Provider name', type=click.Choice(PROVIDERS)) +@click.option('-c', '--cost', required=True, help='Cost (pence) per message including decimals', type=float) @click.option('-d', '--valid_from', required=True, help="Date (%Y-%m-%dT%H:%M:%S) valid from") def create_provider_rates(provider_name, cost, valid_from): """ Backfill rates for a given provider """ - if provider_name not in PROVIDERS: - raise Exception("Invalid provider name, must be one of ({})".format(', '.join(PROVIDERS))) + cost = Decimal(cost) - try: - cost = Decimal(cost) - except: - raise Exception("Invalid cost value.") - - try: - valid_from = datetime.strptime('%Y-%m-%dT%H:%M:%S', valid_from) - except: - raise Exception("Invalid valid_from date. Use the format %Y-%m-%dT%H:%M:%S") + valid_from = datetime.strptime('%Y-%m-%dT%H:%M:%S', valid_from) dao_create_provider_rates(provider_name, valid_from, cost) @@ -203,18 +194,18 @@ def link_inbound_numbers_to_service(): @notify_command() -@click.option('-y', '--year', required=True, help="Use for integer value for year, e.g. 2017") +@click.option('-y', '--year', required=True, help="Use for integer value for year, e.g. 2017", type=int) def populate_monthly_billing(year): """ Populate monthly billing table for all services for a given year. """ def populate(service_id, year, month): - create_or_update_monthly_billing(service_id, datetime(int(year), int(month), 1)) + create_or_update_monthly_billing(service_id, datetime(year, month, 1)) sms_res = get_monthly_billing_by_notification_type( - service_id, datetime(int(year), int(month), 1), SMS_TYPE + service_id, datetime(year, month, 1), SMS_TYPE ) email_res = get_monthly_billing_by_notification_type( - service_id, datetime(int(year), int(month), 1), EMAIL_TYPE + service_id, datetime(year, month, 1), EMAIL_TYPE ) print("Finished populating data for {} for service id {}".format(month, str(service_id))) print('SMS: {}'.format(sms_res.monthly_totals)) @@ -225,7 +216,7 @@ def populate_monthly_billing(year): ) start, end = 1, 13 - if year == '2016': + if year == 2016: start = 4 for service_id in service_ids: @@ -400,7 +391,7 @@ def populate_annual_billing(): @notify_command() -@click.option('-j', '--job_id', required=True, help="Enter the job id to rebuild the dvla file for") +@click.option('-j', '--job_id', required=True, help="Enter the job id to rebuild the dvla file for", type=click.UUID) def re_run_build_dvla_file_for_job(job_id): """ Rebuild dvla file for a job. From 5b7118973ed11b33c82489ac08c79dbb2d20de6b Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 28 Nov 2017 11:10:19 +0000 Subject: [PATCH 2/9] take advantage of click's type validation/coercion (saves us having to write the stuff ourselves). Also adds a small click plugin to do datetime parsing. Sample output: ``` [leohemsted:~/dev/api]$ flask command create_provider_rates --help Usage: flask command create_provider_rates [OPTIONS] Backfill rates for a given provider Options: -p, --provider_name [mmg|firetext|ses] [required] -c, --cost FLOAT Cost (pence) per message including decimals [required] -d, --valid_from DATE [required] --help Show this message and exit. [leohemsted:~/dev/api]$ flask command create_provider_rates -p ses -c 1.234 -d invalid Usage: flask command create_provider_rates [OPTIONS] Error: Invalid value for "-d" / "--valid_from": Could not parse datetime string "invalid" formatted as %Y-%m-%dT%H:%M:%S ``` --- app/commands.py | 16 ++++++---------- requirements.txt | 1 + tests/app/test_commands.py | 2 +- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/app/commands.py b/app/commands.py index 4a8ae4f26..4b8f2b37f 100644 --- a/app/commands.py +++ b/app/commands.py @@ -6,6 +6,7 @@ import functools import flask from flask import current_app import click +from click_datetime import Datetime as click_dt from app import db from app.dao.monthly_billing_dao import ( @@ -49,17 +50,14 @@ class notify_command: @notify_command() -@click.option('-p', '--provider_name', required=True, help='Provider name', type=click.Choice(PROVIDERS)) +@click.option('-p', '--provider_name', required=True, type=click.Choice(PROVIDERS)) @click.option('-c', '--cost', required=True, help='Cost (pence) per message including decimals', type=float) -@click.option('-d', '--valid_from', required=True, help="Date (%Y-%m-%dT%H:%M:%S) valid from") +@click.option('-d', '--valid_from', required=True, type=click_dt(format='%Y-%m-%dT%H:%M:%S')) def create_provider_rates(provider_name, cost, valid_from): """ Backfill rates for a given provider """ cost = Decimal(cost) - - valid_from = datetime.strptime('%Y-%m-%dT%H:%M:%S', valid_from) - dao_create_provider_rates(provider_name, valid_from, cost) @@ -194,7 +192,7 @@ def link_inbound_numbers_to_service(): @notify_command() -@click.option('-y', '--year', required=True, help="Use for integer value for year, e.g. 2017", type=int) +@click.option('-y', '--year', required=True, help="e.g. 2017", type=int) def populate_monthly_billing(year): """ Populate monthly billing table for all services for a given year. @@ -228,14 +226,12 @@ def populate_monthly_billing(year): @notify_command() -@click.option('-s', '--start_date', required=True, help="Date (%Y-%m-%d) start date inclusive") -@click.option('-e', '--end_date', required=True, help="Date (%Y-%m-%d) end date inclusive") +@click.option('-s', '--start_date', required=True, help="start date inclusive", type=click_dt(format='%Y-%m-%d')) +@click.option('-e', '--end_date', required=True, help="end date inclusive", type=click_dt(format='%Y-%m-%d')) def backfill_processing_time(start_date, end_date): """ Send historical performance platform stats. """ - start_date = datetime.strptime(start_date, '%Y-%m-%d') - end_date = datetime.strptime(end_date, '%Y-%m-%d') delta = end_date - start_date diff --git a/requirements.txt b/requirements.txt index c186de052..68937f75d 100644 --- a/requirements.txt +++ b/requirements.txt @@ -7,6 +7,7 @@ Flask-Marshmallow==0.8.0 Flask-Migrate==2.1.1 Flask-SQLAlchemy==2.3.2 Flask==0.12.2 +click-datetime==0.2 gunicorn==19.7.1 iso8601==0.1.12 jsonschema==2.6.0 diff --git a/tests/app/test_commands.py b/tests/app/test_commands.py index d5f6e75e1..d5313d128 100644 --- a/tests/app/test_commands.py +++ b/tests/app/test_commands.py @@ -8,7 +8,7 @@ def test_backfill_processing_time_works_for_correct_dates(mocker, notify_api): # backfill_processing_time is a click.Command object - if you try invoking the callback on its own, it # throws a `RuntimeError: There is no active click context.` - so get at the original function using __wrapped__ - backfill_processing_time.callback.__wrapped__('2017-08-01', '2017-08-03') + backfill_processing_time.callback.__wrapped__(datetime(2017, 8, 1), datetime(2017, 8, 3)) assert send_mock.call_count == 3 send_mock.assert_any_call(datetime(2017, 7, 31, 23, 0), datetime(2017, 8, 1, 23, 0)) From 3ee351b8026ca76e196a1306b2649e94fc8bb08e Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 27 Nov 2017 15:14:36 +0000 Subject: [PATCH 3/9] remove unused db.py and add flake8 config --- .flake8 | 7 +++++++ db.py | 14 -------------- 2 files changed, 7 insertions(+), 14 deletions(-) create mode 100644 .flake8 delete mode 100644 db.py diff --git a/.flake8 b/.flake8 new file mode 100644 index 000000000..bfe5a2603 --- /dev/null +++ b/.flake8 @@ -0,0 +1,7 @@ +[flake8] +# Rule definitions: http://flake8.pycqa.org/en/latest/user/error-codes.html +# W503: line break before binary operator +exclude = venv*,__pycache__,node_modules,cache,migrations +ignore = W503 +max-complexity = 14 +max-line-length = 120 diff --git a/db.py b/db.py deleted file mode 100644 index 7be1aa052..000000000 --- a/db.py +++ /dev/null @@ -1,14 +0,0 @@ -from flask_script import Manager, Server -from flask_migrate import Migrate, MigrateCommand - -from app import create_app, db - - -application = create_app() - -manager = Manager(application) -migrate = Migrate(application, db) -manager.add_command('db', MigrateCommand) - -if __name__ == '__main__': - manager.run() From 043dee5a54bd6da99fa6ccaae74c6ead2b537504 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 27 Nov 2017 15:36:57 +0000 Subject: [PATCH 4/9] ensure no tests share the same name (using flake8's checker) --- .../notification_dao/test_notification_dao.py | 6 ++-- tests/app/dao/test_inbound_numbers_dao.py | 12 ++++--- tests/app/dao/test_jobs_dao.py | 11 ++++--- tests/app/job/test_rest.py | 14 -------- .../notifications/test_post_notifications.py | 33 ++++--------------- 5 files changed, 23 insertions(+), 53 deletions(-) diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index d662b6e4c..4e1ab2945 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -477,7 +477,7 @@ def test_should_not_update_status_by_reference_if_from_country_with_no_delivery_ assert notification.status == NOTIFICATION_SENT -def test_should_not_update_status_by_id_if_sent_to_country_with_no_delivery_receipts(sample_template): +def test_should_not_update_status_by_id_if_sent_to_country_with_carrier_delivery_receipts(sample_template): notification = create_notification( sample_template, status=NOTIFICATION_SENT, @@ -491,7 +491,7 @@ def test_should_not_update_status_by_id_if_sent_to_country_with_no_delivery_rece assert notification.status == NOTIFICATION_SENT -def test_should_not_update_status_by_id_if_sent_to_country_with_no_delivery_receipts(sample_template): +def test_should_not_update_status_by_id_if_sent_to_country_with_delivery_receipts(sample_template): notification = create_notification( sample_template, status=NOTIFICATION_SENT, @@ -2071,7 +2071,7 @@ def test_dao_get_last_notification_added_for_job_id_no_notifications(sample_temp assert dao_get_last_notification_added_for_job_id(job.id) is None -def test_dao_get_last_notification_added_for_job_id_no_notifications(sample_template, fake_uuid): +def test_dao_get_last_notification_added_for_job_id_no_job(sample_template, fake_uuid): assert dao_get_last_notification_added_for_job_id(fake_uuid) is None diff --git a/tests/app/dao/test_inbound_numbers_dao.py b/tests/app/dao/test_inbound_numbers_dao.py index 264c17dc4..ec9a6cc64 100644 --- a/tests/app/dao/test_inbound_numbers_dao.py +++ b/tests/app/dao/test_inbound_numbers_dao.py @@ -93,15 +93,17 @@ def test_dao_allocate_number_for_service(notify_db_session): assert updated_inbound_number.service_id == service.id -def test_dao_allocate_number_for_service(notify_db_session, sample_service): +def test_dao_allocate_number_for_service_raises_if_inbound_number_already_taken(notify_db_session, sample_service): number = '078945612' inbound_number = create_inbound_number(number=number, service_id=sample_service.id) service = create_service(service_name="Service needs an inbound number") - with pytest.raises(Exception): + with pytest.raises(Exception) as exc: dao_allocate_number_for_service(service_id=service.id, inbound_number_id=inbound_number.id) + assert 'is not available' in str(exc.value) -def test_dao_allocate_number_for_service(notify_db_session, sample_service): +def test_dao_allocate_number_for_service_raises_if_invalid_inbound_number(notify_db_session, fake_uuid): service = create_service(service_name="Service needs an inbound number") - with pytest.raises(Exception): - dao_allocate_number_for_service(service_id=service.id, inbound_number_id=uuid.uuid4()) + with pytest.raises(Exception) as exc: + dao_allocate_number_for_service(service_id=service.id, inbound_number_id=fake_uuid) + assert 'is not available' in str(exc.value) diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index 487760ff4..2c16e7ba4 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -426,7 +426,7 @@ def test_should_get_jobs_seven_days_old_filters_type(notify_db, notify_db_sessio assert job_to_remain.id not in [job.id for job in jobs] -def test_dao_get_job_statistics_for_job(notify_db, notify_db_session, sample_job): +def test_dao_get_job_statistics_for_job_calculates_stats(notify_db, notify_db_session, sample_job): notification = create_notification(notify_db=notify_db, notify_db_session=notify_db_session, job=sample_job) notification_delivered = create_notification(notify_db=notify_db, notify_db_session=notify_db_session, job=sample_job, status='delivered') @@ -438,16 +438,19 @@ def test_dao_get_job_statistics_for_job(notify_db, notify_db_session, sample_job create_or_update_job_sending_statistics(notification_failed) update_job_stats_outcome_count(notification_delivered) update_job_stats_outcome_count(notification_failed) + result = dao_get_job_statistics_for_job(sample_job.service_id, sample_job.id) + assert_job_stat(job=sample_job, result=result, sent=3, delivered=1, failed=1) -def test_dao_get_job_statistics_for_job(notify_db, notify_db_session, sample_service): +def test_dao_get_job_statistics_for_job_separates_jobs(notify_db, notify_db_session, sample_service): job_1, job_2 = stats_set_up(notify_db, notify_db_session, sample_service) - result = dao_get_job_statistics_for_job(sample_service.id, job_1.id) - assert_job_stat(job=job_1, result=result, sent=2, delivered=1, failed=0) + result = dao_get_job_statistics_for_job(sample_service.id, job_1.id) result_2 = dao_get_job_statistics_for_job(sample_service.id, job_2.id) + + assert_job_stat(job=job_1, result=result, sent=2, delivered=1, failed=0) assert_job_stat(job=job_2, result=result_2, sent=1, delivered=0, failed=1) diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index f9f0c2929..33f931a09 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -57,20 +57,6 @@ def test_get_job_with_unknown_id_returns404(notify_api, sample_template, fake_uu } -def test_get_job_by_id(notify_api, sample_job): - job_id = str(sample_job.id) - service_id = sample_job.service.id - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = '/service/{}/job/{}'.format(service_id, job_id) - auth_header = create_authorization_header() - response = client.get(path, headers=[auth_header]) - assert response.status_code == 200 - resp_json = json.loads(response.get_data(as_text=True)) - assert resp_json['data']['id'] == job_id - assert resp_json['data']['created_by']['name'] == 'Test User' - - def test_cancel_job(notify_api, sample_scheduled_job): job_id = str(sample_scheduled_job.id) service_id = sample_scheduled_job.service.id diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index f29dbcf49..10ca07c96 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -626,6 +626,12 @@ def test_post_email_notification_with_valid_reply_to_id_returns_201(client, samp assert resp_json['id'] == str(notification.id) assert mocked.called + email_reply_to = NotificationEmailReplyTo.query.one() + + assert email_reply_to.notification_id == notification.id + assert email_reply_to.service_email_reply_to_id == reply_to_email.id + assert notification.reply_to_text == reply_to_email.email_address + def test_post_email_notification_with_invalid_reply_to_id_returns_400(client, sample_email_template, mocker, fake_uuid): mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') @@ -646,33 +652,6 @@ def test_post_email_notification_with_invalid_reply_to_id_returns_400(client, sa assert 'BadRequestError' in resp_json['errors'][0]['error'] -def test_post_email_notification_with_valid_reply_to_id_returns_201(client, sample_email_template, mocker): - reply_to_email = create_reply_to_email(sample_email_template.service, 'test@test.com') - mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - data = { - "email_address": sample_email_template.service.users[0].email_address, - "template_id": sample_email_template.id, - 'email_reply_to_id': reply_to_email.id - } - auth_header = create_authorization_header(service_id=sample_email_template.service_id) - response = client.post( - path="v2/notifications/email", - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - assert response.status_code == 201 - resp_json = json.loads(response.get_data(as_text=True)) - assert validate(resp_json, post_email_response) == resp_json - notification = Notification.query.first() - assert resp_json['id'] == str(notification.id) - assert mocked.called - - email_reply_to = NotificationEmailReplyTo.query.one() - - assert email_reply_to.notification_id == notification.id - assert email_reply_to.service_email_reply_to_id == reply_to_email.id - assert notification.reply_to_text == reply_to_email.email_address - - def test_persist_sender_to_notification_mapping_for_email(notify_db_session): service = create_service() template = create_template(service=service, template_type=EMAIL_TYPE) From 28d5f9b87f3daea47bbaea7d6e9e66d5beb7a2af Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 28 Nov 2017 10:35:16 +0000 Subject: [PATCH 5/9] flake8 - remove unused imports and ensure they're always at the top of the file --- app/__init__.py | 4 ++-- app/authentication/auth.py | 1 - app/billing/billing_schemas.py | 3 --- app/celery/provider_tasks.py | 2 +- app/dao/api_key_dao.py | 2 +- app/dao/notification_usage_dao.py | 1 - app/dao/permissions_dao.py | 3 --- app/dao/service_permissions_dao.py | 2 +- app/dao/service_whitelist_dao.py | 2 +- app/dao/services_dao.py | 1 - app/delivery/rest.py | 5 ++--- app/invite/rest.py | 3 +-- app/job/rest.py | 20 ++++++-------------- app/models.py | 3 +-- app/notifications/receive_notifications.py | 2 +- app/service/statistics.py | 3 +-- app/service/utils.py | 7 ++----- app/template/rest.py | 9 ++++----- app/template_statistics/rest.py | 3 +-- app/user/rest.py | 1 - app/v2/inbound_sms/get_inbound_sms.py | 3 --- run_celery.py | 4 ++-- 22 files changed, 27 insertions(+), 57 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index a1172736e..de0f50aad 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -3,7 +3,7 @@ import random import string import uuid -from flask import Flask, _request_ctx_stack, request, g, jsonify +from flask import _request_ctx_stack, request, g, jsonify from flask_sqlalchemy import SQLAlchemy from flask_marshmallow import Marshmallow from flask_migrate import Migrate @@ -100,7 +100,7 @@ def register_blueprint(application): from app.notifications.receive_notifications import receive_notifications_blueprint from app.notifications.notifications_sms_callback import sms_callback_blueprint from app.notifications.notifications_letter_callback import letter_callback_blueprint - from app.authentication.auth import requires_admin_auth, requires_auth, requires_no_auth, restrict_ip_sms + from app.authentication.auth import requires_admin_auth, requires_auth, requires_no_auth from app.letters.rest import letter_job from app.billing.rest import billing_blueprint diff --git a/app/authentication/auth.py b/app/authentication/auth.py index ae73ec94f..fd77c61b4 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -7,7 +7,6 @@ from sqlalchemy.exc import DataError from sqlalchemy.orm.exc import NoResultFound from app.dao.services_dao import dao_fetch_service_by_id_with_api_keys -from flask import jsonify class AuthError(Exception): diff --git a/app/billing/billing_schemas.py b/app/billing/billing_schemas.py index d5a840184..00d92f1c3 100644 --- a/app/billing/billing_schemas.py +++ b/app/billing/billing_schemas.py @@ -1,6 +1,3 @@ -from app.schema_validation.definitions import uuid, https_url - - create_or_update_free_sms_fragment_limit_schema = { "$schema": "http://json-schema.org/draft-04/schema#", "description": "POST annual billing schema", diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 2400cb83d..5f1f81be1 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -1,4 +1,4 @@ -from celery.signals import worker_process_init, worker_process_shutdown, worker_shutdown +from celery.signals import worker_process_shutdown from flask import current_app from notifications_utils.recipients import InvalidEmailError from sqlalchemy.orm.exc import NoResultFound diff --git a/app/dao/api_key_dao.py b/app/dao/api_key_dao.py index 8d32a10a5..a160a85d3 100644 --- a/app/dao/api_key_dao.py +++ b/app/dao/api_key_dao.py @@ -1,7 +1,7 @@ import uuid from datetime import datetime -from app import db, encryption +from app import db from app.models import ApiKey from app.dao.dao_utils import ( diff --git a/app/dao/notification_usage_dao.py b/app/dao/notification_usage_dao.py index afb547287..5fe278cae 100644 --- a/app/dao/notification_usage_dao.py +++ b/app/dao/notification_usage_dao.py @@ -9,7 +9,6 @@ from app.dao.date_util import get_financial_year from app.models import ( NotificationHistory, Rate, - Service, NOTIFICATION_STATUS_TYPES_BILLABLE, KEY_TYPE_TEST, SMS_TYPE, diff --git a/app/dao/permissions_dao.py b/app/dao/permissions_dao.py index 162d08a46..96c0d56ea 100644 --- a/app/dao/permissions_dao.py +++ b/app/dao/permissions_dao.py @@ -1,10 +1,7 @@ from app import db -from werkzeug.datastructures import MultiDict from app.dao import DAOClass from app.models import ( Permission, - Service, - User, MANAGE_USERS, MANAGE_TEMPLATES, MANAGE_SETTINGS, diff --git a/app/dao/service_permissions_dao.py b/app/dao/service_permissions_dao.py index 3bba3cc0d..4f0c5de22 100644 --- a/app/dao/service_permissions_dao.py +++ b/app/dao/service_permissions_dao.py @@ -1,6 +1,6 @@ from app import db from app.dao.dao_utils import transactional -from app.models import ServicePermission, SERVICE_PERMISSION_TYPES +from app.models import ServicePermission def dao_fetch_service_permissions(service_id): diff --git a/app/dao/service_whitelist_dao.py b/app/dao/service_whitelist_dao.py index b6874d8de..04bbef347 100644 --- a/app/dao/service_whitelist_dao.py +++ b/app/dao/service_whitelist_dao.py @@ -1,5 +1,5 @@ from app import db -from app.models import Service, ServiceWhitelist +from app.models import ServiceWhitelist def dao_fetch_service_whitelist(service_id): diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 851348271..e55775dcc 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -39,7 +39,6 @@ from app.models import ( SMS_TYPE, TEMPLATE_TYPES ) -from app.service.statistics import format_monthly_template_notification_stats from app.statsd_decorators import statsd from app.utils import get_london_month_from_utc_column, get_london_midnight_in_utc from app.dao.annual_billing_dao import dao_insert_annual_billing diff --git a/app/delivery/rest.py b/app/delivery/rest.py index 83236d521..8bc8865b6 100644 --- a/app/delivery/rest.py +++ b/app/delivery/rest.py @@ -1,15 +1,14 @@ -from flask import Blueprint, jsonify +from flask import Blueprint, jsonify, current_app from app.config import QueueNames from app.delivery import send_to_providers from app.models import EMAIL_TYPE from app.celery import provider_tasks from app.dao import notifications_dao -from flask import current_app +from app.errors import register_errors delivery_blueprint = Blueprint('delivery', __name__) -from app.errors import register_errors register_errors(delivery_blueprint) diff --git a/app/invite/rest.py b/app/invite/rest.py index 0553b3a07..55cab2a30 100644 --- a/app/invite/rest.py +++ b/app/invite/rest.py @@ -14,11 +14,10 @@ from app.dao.templates_dao import dao_get_template_by_id from app.models import EMAIL_TYPE, KEY_TYPE_NORMAL, Service from app.notifications.process_notifications import persist_notification, send_notification_to_queue from app.schemas import invited_user_schema +from app.errors import register_errors invite = Blueprint('invite', __name__, url_prefix='/service//invite') -from app.errors import register_errors - register_errors(invite) diff --git a/app/job/rest.py b/app/job/rest.py index 8b2165760..efb67f7c5 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -15,36 +15,28 @@ from app.dao.jobs_dao import ( dao_get_notification_outcomes_for_job, dao_get_job_stats_for_service, dao_get_job_statistics_for_job) - -from app.dao.services_dao import ( - dao_fetch_service_by_id -) - -from app.dao.templates_dao import (dao_get_template_by_id) +from app.dao.services_dao import dao_fetch_service_by_id +from app.dao.templates_dao import dao_get_template_by_id from app.dao.notifications_dao import get_notifications_for_job - from app.schemas import ( job_schema, unarchived_template_schema, notifications_filter_schema, notification_with_template_schema ) - from app.celery.tasks import process_job - from app.models import JOB_STATUS_SCHEDULED, JOB_STATUS_PENDING, JOB_STATUS_CANCELLED, LETTER_TYPE - from app.utils import pagination_links - from app.config import QueueNames - -job_blueprint = Blueprint('job', __name__, url_prefix='/service//job') - from app.errors import ( register_errors, InvalidRequest ) + +job_blueprint = Blueprint('job', __name__, url_prefix='/service//job') + + register_errors(job_blueprint) diff --git a/app/models.py b/app/models.py index a95c09a6f..b930254b4 100644 --- a/app/models.py +++ b/app/models.py @@ -10,8 +10,7 @@ from sqlalchemy.dialects.postgresql import ( UUID, JSON ) -from sqlalchemy import UniqueConstraint, CheckConstraint, and_ -from sqlalchemy.orm import foreign, remote +from sqlalchemy import UniqueConstraint, CheckConstraint from notifications_utils.recipients import ( validate_email_address, validate_phone_number, diff --git a/app/notifications/receive_notifications.py b/app/notifications/receive_notifications.py index 767b7da3c..f44aa17d8 100644 --- a/app/notifications/receive_notifications.py +++ b/app/notifications/receive_notifications.py @@ -4,7 +4,7 @@ import iso8601 from flask import jsonify, Blueprint, current_app, request, abort from notifications_utils.recipients import try_validate_and_format_phone_number -from app import statsd_client, firetext_client, mmg_client +from app import statsd_client from app.celery import tasks from app.config import QueueNames from app.dao.services_dao import dao_fetch_service_by_inbound_number diff --git a/app/service/statistics.py b/app/service/statistics.py index 8ebf8434b..26b3fb289 100644 --- a/app/service/statistics.py +++ b/app/service/statistics.py @@ -1,5 +1,4 @@ -import itertools -from datetime import datetime, timedelta +from datetime import datetime from app.models import NOTIFICATION_STATUS_TYPES, TEMPLATE_TYPES diff --git a/app/service/utils.py b/app/service/utils.py index 1d89d4fb6..d25c6b7de 100644 --- a/app/service/utils.py +++ b/app/service/utils.py @@ -1,15 +1,12 @@ import itertools -from app.dao.date_util import get_financial_year +from notifications_utils.recipients import allowed_to_send_to + from app.models import ( ServiceWhitelist, MOBILE_TYPE, EMAIL_TYPE, KEY_TYPE_TEST, KEY_TYPE_TEAM, KEY_TYPE_NORMAL) -from notifications_utils.recipients import allowed_to_send_to - -from datetime import datetime - def get_recipients_from_request(request_json, key, type): return [(type, recipient) for recipient in request_json.get(key)] diff --git a/app/template/rest.py b/app/template/rest.py index de42e723d..1f869fb5d 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -15,19 +15,18 @@ from app.dao.templates_dao import ( ) from notifications_utils.template import SMSMessageTemplate from app.dao.services_dao import dao_fetch_service_by_id -from app.dao.service_permissions_dao import dao_fetch_service_permissions -from app.models import SMS_TYPE, EMAIL_TYPE +from app.models import SMS_TYPE from app.notifications.validators import service_has_permission from app.schemas import (template_schema, template_history_schema) - -template_blueprint = Blueprint('template', __name__, url_prefix='/service//template') - from app.errors import ( register_errors, InvalidRequest ) from app.utils import get_template_instance, get_public_notify_type_text +template_blueprint = Blueprint('template', __name__, url_prefix='/service//template') + + register_errors(template_blueprint) diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index 67b6e72d1..86548dc00 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -16,13 +16,12 @@ from app.dao.templates_dao import ( from app.schemas import notification_with_template_schema from app.utils import cache_key_for_service_template_counter +from app.errors import register_errors, InvalidRequest template_statistics = Blueprint('template-statistics', __name__, url_prefix='/service//template-statistics') -from app.errors import register_errors, InvalidRequest - register_errors(template_statistics) diff --git a/app/user/rest.py b/app/user/rest.py index 4b4e9c943..87e9cf305 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -33,7 +33,6 @@ from app.schemas import ( email_data_request_schema, user_schema, permission_schema, - user_schema_load_json, user_update_schema_load_json, user_update_password_schema_load_json ) diff --git a/app/v2/inbound_sms/get_inbound_sms.py b/app/v2/inbound_sms/get_inbound_sms.py index 478e4f03d..f4225a011 100644 --- a/app/v2/inbound_sms/get_inbound_sms.py +++ b/app/v2/inbound_sms/get_inbound_sms.py @@ -1,8 +1,5 @@ from flask import jsonify, request, url_for, current_app -from notifications_utils.recipients import validate_and_format_phone_number -from notifications_utils.recipients import InvalidPhoneError - from app import authenticated_service from app.dao import inbound_sms_dao from app.schema_validation import validate diff --git a/run_celery.py b/run_celery.py index 4fb28ae08..0b847892e 100644 --- a/run_celery.py +++ b/run_celery.py @@ -1,8 +1,8 @@ #!/usr/bin/env python -# notify_celery is referenced from manifest_delivery_base.yml, and cannot be removed from flask import Flask -from app import notify_celery, create_app +# notify_celery is referenced from manifest_delivery_base.yml, and cannot be removed +from app import notify_celery, create_app # noqa application = Flask('delivery') From 28088428f19e7f738ebcf03f59d7bb39c5983d79 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 28 Nov 2017 13:49:14 +0000 Subject: [PATCH 6/9] flake8 - misc flake8 errs. * unused variables * variables in loops overshadowing imports * excepts with no defined exc type (tried to avoid `except Exception` too) * history mapper is still too complex * default variables should never be mutable --- app/dao/annual_billing_dao.py | 2 +- app/dao/services_dao.py | 8 ++++---- app/delivery/send_to_providers.py | 3 ++- app/history_meta.py | 2 +- app/schemas.py | 6 +++--- app/service/sender.py | 4 +++- 6 files changed, 14 insertions(+), 11 deletions(-) diff --git a/app/dao/annual_billing_dao.py b/app/dao/annual_billing_dao.py index 37361d7b1..76a4e2f46 100644 --- a/app/dao/annual_billing_dao.py +++ b/app/dao/annual_billing_dao.py @@ -35,7 +35,7 @@ def dao_update_annual_billing_for_current_and_future_years(service_id, free_sms_ if not financial_year_start: financial_year_start = get_current_financial_year_start_year() - updated = AnnualBilling.query.filter( + AnnualBilling.query.filter( AnnualBilling.service_id == service_id, AnnualBilling.financial_year_start >= financial_year_start ).update( diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index e55775dcc..8753b2dc4 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -318,22 +318,22 @@ def dao_fetch_monthly_historical_stats_for_service(service_id, year): ) months = { - datetime.strftime(date, '%Y-%m'): { + datetime.strftime(created_date, '%Y-%m'): { template_type: dict.fromkeys( NOTIFICATION_STATUS_TYPES, 0 ) for template_type in TEMPLATE_TYPES } - for date in [ + for created_date in [ datetime(year, month, 1) for month in range(4, 13) ] + [ datetime(year + 1, month, 1) for month in range(1, 4) ] } - for notification_type, status, date, count in rows: - months[datetime.strftime(date, "%Y-%m")][notification_type][status] = count + for notification_type, status, created_date, count in rows: + months[datetime.strftime(created_date, "%Y-%m")][notification_type][status] = count return months diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 5ec949fec..cbcfd7650 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -7,6 +7,7 @@ from notifications_utils.recipients import ( validate_and_format_email_address ) from notifications_utils.template import HTMLEmailTemplate, PlainTextEmailTemplate, SMSMessageTemplate +from requests.exceptions import HTTPError from app import clients, statsd_client, create_uuid from app.dao.notifications_dao import ( @@ -60,7 +61,7 @@ def send_sms_to_provider(notification): update_notification(notification, provider) try: send_sms_response(provider.get_name(), str(notification.id), notification.to) - except: + except HTTPError: # when we retry, we only do anything if the notification is in created - it's currently in sending, # so set it back so that we actually attempt the callback again notification.sent_at = None diff --git a/app/history_meta.py b/app/history_meta.py index ff44255f9..d49c39e8a 100644 --- a/app/history_meta.py +++ b/app/history_meta.py @@ -33,7 +33,7 @@ def _is_versioning_col(col): return "version_meta" in col.info -def _history_mapper(local_mapper): +def _history_mapper(local_mapper): # noqa (C901 too complex) cls = local_mapper.class_ # set the "active_history" flag diff --git a/app/schemas.py b/app/schemas.py index b4d34ab62..419c9b438 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -33,9 +33,9 @@ from app.utils import get_template_instance def _validate_positive_number(value, msg="Not a positive integer"): try: page_int = int(value) - if page_int < 1: - raise ValidationError(msg) - except: + except ValueError: + raise ValidationError(msg) + if page_int < 1: raise ValidationError(msg) diff --git a/app/service/sender.py b/app/service/sender.py index e87213a7b..7052cbfb2 100644 --- a/app/service/sender.py +++ b/app/service/sender.py @@ -7,7 +7,9 @@ from app.models import EMAIL_TYPE, KEY_TYPE_NORMAL from app.notifications.process_notifications import persist_notification, send_notification_to_queue -def send_notification_to_service_users(service_id, template_id, personalisation={}, include_user_fields=[]): +def send_notification_to_service_users(service_id, template_id, personalisation=None, include_user_fields=None): + personalisation = personalisation or {} + include_user_fields = include_user_fields or [] template = dao_get_template_by_id(template_id) service = dao_fetch_service_by_id(service_id) active_users = dao_fetch_active_users_for_service(service.id) From 90e9a2f1b3fdf6670d9001d1d3e3010081d9d142 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 28 Nov 2017 13:51:47 +0000 Subject: [PATCH 7/9] use flake8 instead of pycodestyle since there are thousands and thousands of errors in the tests files at the moment, i propose fixing those errors in separate PR for now. --- README.md | 2 +- requirements_for_test.txt | 4 ++-- scripts/delete_sqs_queues.py | 1 + scripts/run_tests.sh | 3 ++- setup.cfg | 5 ----- 5 files changed, 6 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index dce3dbad6..8fba7f298 100644 --- a/README.md +++ b/README.md @@ -100,7 +100,7 @@ Then simply run make test ``` -That will run pycodestyle for code analysis and our unit test suite. If you wish to run our functional tests, instructions can be found in the +That will run flake8 for code analysis and our unit test suite. If you wish to run our functional tests, instructions can be found in the [notifications-functional-tests](https://github.com/alphagov/notifications-functional-tests) repository. diff --git a/requirements_for_test.txt b/requirements_for_test.txt index af0beb814..ae83bf250 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -1,5 +1,5 @@ -r requirements.txt -pycodestyle==2.3.1 +flake8==3.5.0 pytest==3.2.5 pytest-mock==1.6.3 pytest-cov==2.5.1 @@ -9,4 +9,4 @@ freezegun==0.3.9 requests-mock==1.3.0 # optional requirements for jsonschema strict-rfc3339==0.7 -rfc3987==1.3.7 \ No newline at end of file +rfc3987==1.3.7 diff --git a/scripts/delete_sqs_queues.py b/scripts/delete_sqs_queues.py index b167ce392..17036adb5 100755 --- a/scripts/delete_sqs_queues.py +++ b/scripts/delete_sqs_queues.py @@ -64,6 +64,7 @@ def delete_queue(queue_url): print('Deleted queue successfully {}'.format(response['ResponseMetadata'])) else: print('Error occured when attempting to delete queue') + from pprint import pprint pprint(response) return response diff --git a/scripts/run_tests.sh b/scripts/run_tests.sh index ece4a6283..d58c98b22 100755 --- a/scripts/run_tests.sh +++ b/scripts/run_tests.sh @@ -27,7 +27,8 @@ function display_result { if [[ -z "$VIRTUAL_ENV" ]] && [[ -d venv ]]; then source ./venv/bin/activate fi -pycodestyle . +echo -e "\033[31mWARNING. NOT RUNNING flake8 AGAINST TEST DIRECTORY DUE TO LARGE AMOUNT OF EXISTING ISSUES.\033[0m" +flake8 app/ display_result $? 1 "Code style check" # run with four concurrent threads diff --git a/setup.cfg b/setup.cfg index 60399a147..4db8682ef 100644 --- a/setup.cfg +++ b/setup.cfg @@ -1,7 +1,2 @@ -[pycodestyle] -max-line-length = 120 -ignore = E402, E722 -exclude = ./migrations,./venv,./venv3,./build,./cache - [tool:pytest] xfail_strict=true From 5761d228222c43314e6ac81a258c820f53e2b0f4 Mon Sep 17 00:00:00 2001 From: venusbb Date: Tue, 28 Nov 2017 15:25:15 +0000 Subject: [PATCH 8/9] Migration to create new database table service_callback_api --- app/models.py | 34 ++++++++++++ ...o.py => 0145_add_notification_reply_to.py} | 0 .../versions/0146_add_service_callback_api.py | 55 +++++++++++++++++++ 3 files changed, 89 insertions(+) rename migrations/versions/{0144_add_notification_reply_to.py => 0145_add_notification_reply_to.py} (100%) create mode 100644 migrations/versions/0146_add_service_callback_api.py diff --git a/app/models.py b/app/models.py index a95c09a6f..fed090c24 100644 --- a/app/models.py +++ b/app/models.py @@ -466,6 +466,40 @@ class ServiceInboundApi(db.Model, Versioned): } +class ServiceCallbackApi(db.Model, Versioned): + __tablename__ = 'service_callback_api' + id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) + service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, nullable=False, unique=True) + service = db.relationship('Service', backref='service_callback_api') + url = db.Column(db.String(), nullable=False) + _bearer_token = db.Column("bearer_token", db.String(), nullable=False) + created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow, nullable=False) + updated_at = db.Column(db.DateTime, nullable=True) + updated_by = db.relationship('User') + updated_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) + + @property + def bearer_token(self): + if self._bearer_token: + return encryption.decrypt(self._bearer_token) + return None + + @bearer_token.setter + def bearer_token(self, bearer_token): + if bearer_token: + self._bearer_token = encryption.encrypt(str(bearer_token)) + + def serialize(self): + return { + "id": str(self.id), + "service_id": str(self.service_id), + "url": self.url, + "updated_by_id": str(self.updated_by_id), + "created_at": self.created_at.strftime(DATETIME_FORMAT), + "updated_at": self.updated_at.strftime(DATETIME_FORMAT) if self.updated_at else None + } + + class ApiKey(db.Model, Versioned): __tablename__ = 'api_keys' diff --git a/migrations/versions/0144_add_notification_reply_to.py b/migrations/versions/0145_add_notification_reply_to.py similarity index 100% rename from migrations/versions/0144_add_notification_reply_to.py rename to migrations/versions/0145_add_notification_reply_to.py diff --git a/migrations/versions/0146_add_service_callback_api.py b/migrations/versions/0146_add_service_callback_api.py new file mode 100644 index 000000000..07368d3f8 --- /dev/null +++ b/migrations/versions/0146_add_service_callback_api.py @@ -0,0 +1,55 @@ +""" + +Revision ID: 0146_add_service_callback_api +Revises: 0145_add_notification_reply_to +Create Date: 2017-11-28 15:13:48.730554 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0146_add_service_callback_api' +down_revision = '0145_add_notification_reply_to' + + +def upgrade(): + op.create_table('service_callback_api_history', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('service_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('url', sa.String(), nullable=False), + sa.Column('bearer_token', sa.String(), nullable=False), + sa.Column('created_at', sa.DateTime(), nullable=False), + sa.Column('updated_at', sa.DateTime(), nullable=True), + sa.Column('updated_by_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('version', sa.Integer(), autoincrement=False, nullable=False), + sa.PrimaryKeyConstraint('id', 'version') + ) + op.create_index(op.f('ix_service_callback_api_history_service_id'), 'service_callback_api_history', + ['service_id'], unique=False) + op.create_index(op.f('ix_service_callback_api_history_updated_by_id'), 'service_callback_api_history', + ['updated_by_id'], unique=False) + op.create_table('service_callback_api', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('service_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('url', sa.String(), nullable=False), + sa.Column('bearer_token', sa.String(), nullable=False), + sa.Column('created_at', sa.DateTime(), nullable=False), + sa.Column('updated_at', sa.DateTime(), nullable=True), + sa.Column('updated_by_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('version', sa.Integer(), nullable=False), + sa.ForeignKeyConstraint(['service_id'], ['services.id'], ), + sa.ForeignKeyConstraint(['updated_by_id'], ['users.id'], ), + sa.PrimaryKeyConstraint('id') + ) + op.create_index(op.f('ix_service_callback_api_service_id'), 'service_callback_api', ['service_id'], unique=True) + op.create_index(op.f('ix_service_callback_api_updated_by_id'), 'service_callback_api', ['updated_by_id'], unique=False) + + +def downgrade(): + op.drop_index(op.f('ix_service_callback_api_updated_by_id'), table_name='service_callback_api') + op.drop_index(op.f('ix_service_callback_api_service_id'), table_name='service_callback_api') + op.drop_table('service_callback_api') + op.drop_index(op.f('ix_service_callback_api_history_updated_by_id'), table_name='service_callback_api_history') + op.drop_index(op.f('ix_service_callback_api_history_service_id'), table_name='service_callback_api_history') + op.drop_table('service_callback_api_history') From 349887178c76a08d500e86318868fc8c1b9b22dc Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Wed, 29 Nov 2017 14:13:26 +0000 Subject: [PATCH 9/9] Increased Gunicorn threads Increased Gunicorn threads to 10 on staging to test if there is a performance increase when testing request per second. Increased to 10. Gunicorn recommend 2 x cores + 1 however on PaaS the number of cores is not consistent. --- manifest-api-staging.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/manifest-api-staging.yml b/manifest-api-staging.yml index 67044f918..d19455470 100644 --- a/manifest-api-staging.yml +++ b/manifest-api-staging.yml @@ -1,7 +1,7 @@ --- inherit: manifest-api-base.yml - +command: scripts/run_app_paas.sh gunicorn -c /home/vcap/app/gunicorn_config.py --error-logfile /home/vcap/logs/gunicorn_error.log -w 10 -b 0.0.0.0:$PORT application services: - notify-aws - notify-config