diff --git a/app/__init__.py b/app/__init__.py index 62fe06c50..255af272d 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -268,10 +268,6 @@ def register_v2_blueprints(application): def init_app(app): - @app.before_request - def record_user_agent(): - statsd_client.incr("user-agent.{}".format(process_user_agent(request.headers.get('User-Agent', None)))) - @app.before_request def record_request_details(): CONCURRENT_REQUESTS.inc() @@ -318,18 +314,6 @@ def create_random_identifier(): return ''.join(random.choice(string.ascii_uppercase + string.digits) for _ in range(16)) -def process_user_agent(user_agent_string): - if user_agent_string and user_agent_string.lower().startswith("notify"): - components = user_agent_string.split("/") - client_name = components[0].lower() - client_version = components[1].replace(".", "-") - return "{}.{}".format(client_name, client_version) - elif user_agent_string and not user_agent_string.lower().startswith("notify"): - return "non-notify-user-agent" - else: - return "unknown" - - def setup_sqlalchemy_events(app): TOTAL_DB_CONNECTIONS = Gauge( diff --git a/app/commands.py b/app/commands.py index 766bc9f2d..8002e1c02 100644 --- a/app/commands.py +++ b/app/commands.py @@ -773,9 +773,24 @@ def get_letter_details_from_zips_sent_file(file_paths): rows_from_file.extend(json.loads(file_contents)) notification_references = tuple(row[18:34] for row in rows_from_file) + get_letters_data_from_references(notification_references) + +@notify_command(name='get-notification-and-service-ids-for-letters-that-failed-to-print') +@click.option('-f', '--file_name', required=True, + help="""Full path of the file to upload, file should contain letter filenames, one per line""") +def get_notification_and_service_ids_for_letters_that_failed_to_print(file_name): + print("Getting service and notification ids for letter filenames list {}".format(file_name)) + file = open(file_name) + references = tuple([row[7:23] for row in file]) + + get_letters_data_from_references(tuple(references)) + file.close() + + +def get_letters_data_from_references(notification_references): sql = """ - SELECT id, service_id, reference, job_id, created_at + SELECT id, service_id, template_id, reference, job_id, created_at FROM notifications WHERE reference IN :notification_references ORDER BY service_id, job_id""" @@ -783,7 +798,7 @@ def get_letter_details_from_zips_sent_file(file_paths): with open('zips_sent_details.csv', 'w') as csvfile: csv_writer = csv.writer(csvfile) - csv_writer.writerow(['notification_id', 'service_id', 'reference', 'job_id', 'created_at']) + csv_writer.writerow(['notification_id', 'service_id', 'template_id', 'reference', 'job_id', 'created_at']) for row in result: csv_writer.writerow(row) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 8b0b67568..8bc62657e 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -77,7 +77,6 @@ def dao_get_last_date_template_was_used(template_id, service_id): return last_date -@statsd(namespace="dao") @transactional def dao_create_notification(notification): if not notification.id: diff --git a/app/schemas.py b/app/schemas.py index a44514b5d..a2601ed75 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -63,9 +63,16 @@ class UUIDsAsStringsMixin: @post_dump() def __post_dump(self, data): for key, value in data.items(): + if isinstance(value, UUID): data[key] = str(value) + if isinstance(value, list): + data[key] = [ + (str(item) if isinstance(item, UUID) else item) + for item in value + ] + class BaseSchema(ma.ModelSchema): @@ -249,7 +256,6 @@ class ServiceSchema(BaseSchema, UUIDsAsStringsMixin): 'inbound_number', 'inbound_sms', 'letter_logo_filename', - 'rate_limit', 'returned_letters', 'users', 'version', @@ -350,9 +356,7 @@ class BaseTemplateSchema(BaseSchema): class TemplateSchema(BaseTemplateSchema, UUIDsAsStringsMixin): - created_by_id = field_for( - models.Template, 'created_by_id', dump_to='created_by', dump_only=True - ) + created_by = field_for(models.Template, 'created_by', required=True) process_type = field_for(models.Template, 'process_type') redact_personalisation = fields.Method("redact") @@ -366,9 +370,6 @@ class TemplateSchema(BaseTemplateSchema, UUIDsAsStringsMixin): if not subject or subject.strip() == '': raise ValidationError('Invalid template subject', 'subject') - class Meta(BaseTemplateSchema.Meta): - exclude = BaseTemplateSchema.Meta.exclude + ('created_by',) - class TemplateSchemaNoDetail(TemplateSchema): class Meta(TemplateSchema.Meta): @@ -376,6 +377,7 @@ class TemplateSchemaNoDetail(TemplateSchema): 'archived', 'content', 'created_at', + 'created_by', 'created_by_id', 'hidden', 'postage', diff --git a/app/serialised_models.py b/app/serialised_models.py index e47c8800c..2b3af3903 100644 --- a/app/serialised_models.py +++ b/app/serialised_models.py @@ -120,7 +120,9 @@ class SerialisedService(SerialisedModel): 'active', 'contact_link', 'email_from', + 'message_limit', 'permissions', + 'rate_limit', 'research_mode', 'restricted', } diff --git a/requirements-app.txt b/requirements-app.txt index 444164308..58add7c6c 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -27,7 +27,7 @@ notifications-python-client==5.5.1 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@39.7.1#egg=notifications-utils==39.7.1 +git+https://github.com/alphagov/notifications-utils.git@40.0.0#egg=notifications-utils==40.0.0 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.7.1 diff --git a/requirements.txt b/requirements.txt index c70959d8c..3d1087c14 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,7 +29,7 @@ notifications-python-client==5.5.1 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@39.7.1#egg=notifications-utils==39.7.1 +git+https://github.com/alphagov/notifications-utils.git@40.0.0#egg=notifications-utils==40.0.0 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.7.1 @@ -40,14 +40,14 @@ alembic==1.4.2 amqp==1.4.9 anyjson==0.3.3 attrs==19.3.0 -awscli==1.18.85 +awscli==1.18.89 bcrypt==3.1.7 billiard==3.3.0.23 bleach==3.1.4 blinker==1.4 boto==2.49.0 boto3==1.10.38 -botocore==1.17.8 +botocore==1.17.12 certifi==2020.6.20 chardet==3.0.4 click==7.1.2 @@ -58,8 +58,8 @@ flask-redis==0.4.0 future==0.18.2 govuk-bank-holidays==0.6 greenlet==0.4.16 -idna==2.9 -importlib-metadata==1.6.1 +idna==2.10 +importlib-metadata==1.7.0 Jinja2==2.11.2 jmespath==0.10.0 kombu==3.0.37 diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index ce3b0ce7a..4e6748917 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -23,7 +23,7 @@ from app.notifications.validators import ( validate_and_format_recipient, validate_template, ) -from app.serialised_models import SerialisedService, SerialisedTemplate +from app.serialised_models import SerialisedService, SerialisedTemplate, SerialisedAPIKeyCollection from app.utils import get_template_instance from app.v2.errors import ( @@ -42,6 +42,7 @@ from tests.app.db import ( create_service_whitelist, create_template, ) +from unittest.mock import ANY # all of these tests should have redis enabled (except where we specifically disable it) @@ -56,12 +57,20 @@ def test_check_service_message_limit_in_cache_with_unrestricted_service_is_allow key_type, sample_service, mocker): - mocker.patch('app.notifications.validators.redis_store.get', return_value=1) + mocker.patch('app.notifications.validators.redis_store.get', side_effect=[ + None, # The serialised service + 1, # The rolling count + ]) mocker.patch('app.notifications.validators.redis_store.set') mocker.patch('app.notifications.validators.services_dao') + serialised_service = SerialisedService.from_id(sample_service.id) - check_service_over_daily_message_limit(key_type, sample_service) - app.notifications.validators.redis_store.set.assert_not_called() + check_service_over_daily_message_limit(key_type, serialised_service) + app.notifications.validators.redis_store.set.assert_called_once_with( + f'service-{serialised_service.id}', + ANY, + ex=ANY, + ) assert not app.notifications.validators.services_dao.mock_calls @@ -70,17 +79,27 @@ def test_check_service_message_limit_in_cache_under_message_limit_passes( key_type, sample_service, mocker): - mocker.patch('app.notifications.validators.redis_store.get', return_value=1) + mocker.patch('app.notifications.validators.redis_store.get', side_effect=[ + None, # The serialised service + 1, # The rolling count + ]) mocker.patch('app.notifications.validators.redis_store.set') mocker.patch('app.notifications.validators.services_dao') - check_service_over_daily_message_limit(key_type, sample_service) - app.notifications.validators.redis_store.set.assert_not_called() + serialised_service = SerialisedService.from_id(sample_service.id) + check_service_over_daily_message_limit(key_type, serialised_service) + app.notifications.validators.redis_store.set.assert_called_once_with( + f'service-{serialised_service.id}', + ANY, + ex=ANY, + ) assert not app.notifications.validators.services_dao.mock_calls def test_should_not_interact_with_cache_for_test_key(sample_service, mocker): mocker.patch('app.notifications.validators.redis_store') - check_service_over_daily_message_limit('test', sample_service) + mocker.patch('app.notifications.validators.redis_store.get', side_effect=[None]) + serialised_service = SerialisedService.from_id(sample_service.id) + check_service_over_daily_message_limit('test', serialised_service) assert not app.notifications.validators.redis_store.mock_calls @@ -91,22 +110,24 @@ def test_should_set_cache_value_as_value_from_database_if_cache_not_set( sample_service, mocker ): + serialised_service = SerialisedService.from_id(sample_service.id) with freeze_time("2016-01-01 12:00:00.000000"): for x in range(5): create_notification(sample_template) mocker.patch('app.notifications.validators.redis_store.get', return_value=None) mocker.patch('app.notifications.validators.redis_store.set') - check_service_over_daily_message_limit(key_type, sample_service) + check_service_over_daily_message_limit(key_type, serialised_service) app.notifications.validators.redis_store.set.assert_called_with( str(sample_service.id) + "-2016-01-01-count", 5, ex=3600 ) def test_should_not_access_database_if_redis_disabled(notify_api, sample_service, mocker): + serialised_service = SerialisedService.from_id(sample_service.id) with set_config(notify_api, 'REDIS_ENABLED', False): db_mock = mocker.patch('app.notifications.validators.services_dao') - check_service_over_daily_message_limit('normal', sample_service) + check_service_over_daily_message_limit('normal', serialised_service) assert db_mock.method_calls == [] @@ -120,11 +141,12 @@ def test_check_service_message_limit_over_message_limit_fails(key_type, sample_s sample_service.restricted = True sample_service.message_limit = 4 template = create_template(sample_service) + serialised_service = SerialisedService.from_id(sample_service.id) for x in range(5): create_notification(template) with pytest.raises(TooManyRequestsError) as e: - check_service_over_daily_message_limit(key_type, sample_service) + check_service_over_daily_message_limit(key_type, serialised_service) assert e.value.status_code == 429 assert e.value.message == 'Exceeded send limits (4) for today' assert e.value.fields == [] @@ -139,17 +161,25 @@ def test_check_service_message_limit_in_cache_over_message_limit_fails( key_type, mocker): with freeze_time("2016-01-01 12:00:00.000000"): - mocker.patch('app.redis_store.get', return_value=5) + mocker.patch('app.redis_store.get', side_effect=[ + None, # The serialised service + 5, # The rolling count + ]) mocker.patch('app.notifications.validators.redis_store.set') mocker.patch('app.notifications.validators.services_dao') service = create_service(restricted=True, message_limit=4) + serialised_service = SerialisedService.from_id(service.id) with pytest.raises(TooManyRequestsError) as e: - check_service_over_daily_message_limit(key_type, service) + check_service_over_daily_message_limit(key_type, serialised_service) assert e.value.status_code == 429 assert e.value.message == 'Exceeded send limits (4) for today' assert e.value.fields == [] - app.notifications.validators.redis_store.set.assert_not_called() + app.notifications.validators.redis_store.set.assert_called_once_with( + f'service-{serialised_service.id}', + ANY, + ex=ANY, + ) assert not app.notifications.validators.services_dao.mock_calls @@ -194,23 +224,25 @@ def test_check_template_is_active_fails(sample_template): ['test', 'normal']) def test_service_can_send_to_recipient_passes(key_type, notify_db_session): trial_mode_service = create_service(service_name='trial mode', restricted=True) + serialised_service = SerialisedService.from_id(trial_mode_service.id) assert service_can_send_to_recipient(trial_mode_service.users[0].email_address, key_type, - trial_mode_service) is None + serialised_service) is None assert service_can_send_to_recipient(trial_mode_service.users[0].mobile_number, key_type, - trial_mode_service) is None + serialised_service) is None @pytest.mark.parametrize('key_type', ['test', 'normal']) def test_service_can_send_to_recipient_passes_for_live_service_non_team_member(key_type, sample_service): + serialised_service = SerialisedService.from_id(sample_service.id) assert service_can_send_to_recipient("some_other_email@test.com", key_type, - sample_service) is None + serialised_service) is None assert service_can_send_to_recipient('07513332413', key_type, - sample_service) is None + serialised_service) is None def test_service_can_send_to_recipient_passes_for_whitelisted_recipient_passes(sample_service): @@ -383,9 +415,11 @@ def test_that_when_exceed_rate_limit_request_fails( sample_service.restricted = True api_key = create_api_key(sample_service, key_type=api_key_type) + serialised_service = SerialisedService.from_id(sample_service.id) + serialised_api_key = SerialisedAPIKeyCollection.from_service_id(serialised_service.id)[0] with pytest.raises(RateLimitError) as e: - check_service_over_api_rate_limit(sample_service, api_key) + check_service_over_api_rate_limit(serialised_service, serialised_api_key) assert app.redis_store.exceeded_rate_limit.called_with( "{}-{}".format(str(sample_service.id), api_key.key_type), @@ -408,8 +442,10 @@ def test_that_when_not_exceeded_rate_limit_request_succeeds( sample_service.restricted = True api_key = create_api_key(sample_service) + serialised_service = SerialisedService.from_id(sample_service.id) + serialised_api_key = SerialisedAPIKeyCollection.from_service_id(serialised_service.id)[0] - check_service_over_api_rate_limit(sample_service, api_key) + check_service_over_api_rate_limit(serialised_service, serialised_api_key) assert app.redis_store.exceeded_rate_limit.called_with( "{}-{}".format(str(sample_service.id), api_key.key_type), 3000, @@ -427,9 +463,11 @@ def test_should_not_rate_limit_if_limiting_is_disabled( mocker.patch('app.notifications.validators.services_dao') sample_service.restricted = True - api_key = create_api_key(sample_service) + create_api_key(sample_service) + serialised_service = SerialisedService.from_id(sample_service.id) + serialised_api_key = SerialisedAPIKeyCollection.from_service_id(serialised_service.id)[0] - check_service_over_api_rate_limit(sample_service, api_key) + check_service_over_api_rate_limit(serialised_service, serialised_api_key) assert not app.redis_store.exceeded_rate_limit.called diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index bd1183110..867f66b29 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -233,8 +233,34 @@ def test_get_service_by_id(admin_request, sample_service): assert json_resp['data']['id'] == str(sample_service.id) assert not json_resp['data']['research_mode'] assert json_resp['data']['email_branding'] is None - assert 'branding' not in json_resp['data'] assert json_resp['data']['prefix_sms'] is True + assert json_resp['data'].keys() == { + 'active', + 'consent_to_research', + 'contact_link', + 'count_as_live', + 'created_by', + 'email_branding', + 'email_from', + 'go_live_at', + 'go_live_user', + 'id', + 'inbound_api', + 'letter_branding', + 'message_limit', + 'name', + 'organisation', + 'organisation_type', + 'permissions', + 'prefix_sms', + 'rate_limit', + 'research_mode', + 'restricted', + 'service_callback_api', + 'volume_email', + 'volume_letter', + 'volume_sms', + } @pytest.mark.parametrize('detailed', [True, False]) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 5b49a8cd7..59f9fbff3 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -340,8 +340,13 @@ def test_must_have_a_subject_on_an_email_or_letter_template(client, sample_user, def test_update_should_update_a_template(client, sample_user): + service = create_service(service_permissions=[LETTER_TYPE]) template = create_template(service, template_type="letter", postage="second") + + assert template.created_by == service.created_by + assert template.created_by != sample_user + data = { 'content': 'my template has new content, swell!', 'created_by': str(sample_user.id), @@ -366,6 +371,14 @@ def test_update_should_update_a_template(client, sample_user): assert update_json_resp['data']['template_type'] == template.template_type assert update_json_resp['data']['version'] == 2 + assert update_json_resp['data']['created_by'] == str(sample_user.id) + assert [ + template.created_by_id for template in TemplateHistory.query.all() + ] == [ + service.created_by.id, + sample_user.id, + ] + def test_should_be_able_to_archive_template(client, sample_template): data = { diff --git a/tests/app/test_user_agent_processing.py b/tests/app/test_user_agent_processing.py deleted file mode 100644 index 161b0e475..000000000 --- a/tests/app/test_user_agent_processing.py +++ /dev/null @@ -1,13 +0,0 @@ -from app import process_user_agent - - -def test_can_process_notify_api_user_agent(): - assert "notify-api-python-client.3-0-0" == process_user_agent("NOTIFY-API-PYTHON-CLIENT/3.0.0") - - -def test_can_handle_non_notify_api_user_agent(): - assert "non-notify-user-agent" == process_user_agent("Mozilla/5.0 (iPad; U; CPU OS 3_2_1 like Mac OS X; en-us) AppleWebKit/531.21.10 (KHTML, like Gecko) Mobile/7B405") # noqa - - -def test_handles_null_user_agent(): - assert "unknown" == process_user_agent(None) diff --git a/tests/app/v2/inbound_sms/test_get_inbound_sms.py b/tests/app/v2/inbound_sms/test_get_inbound_sms.py index dfd4ae0b9..7231ed9b9 100644 --- a/tests/app/v2/inbound_sms/test_get_inbound_sms.py +++ b/tests/app/v2/inbound_sms/test_get_inbound_sms.py @@ -1,7 +1,7 @@ from flask import json, url_for from tests import create_authorization_header -from tests.app.db import create_inbound_sms +from tests.app.db import create_inbound_sms, create_service_inbound_api, create_service_callback_api def test_get_inbound_sms_returns_200( @@ -31,6 +31,27 @@ def test_get_inbound_sms_returns_200( assert json_response == expected_response +def test_get_inbound_sms_returns_200_when_service_has_callbacks( + client, sample_service +): + create_service_inbound_api( + service=sample_service, + url="https://inbound.example.com", + ) + create_service_callback_api( + service=sample_service, + url="https://inbound.example.com", + ) + + auth_header = create_authorization_header(service_id=sample_service.id) + response = client.get( + path='/v2/received-text-messages', + headers=[('Content-Type', 'application/json'), auth_header], + ) + + assert response.status_code == 200 + + def test_get_inbound_sms_generate_page_links(client, sample_service, mocker): mocker.patch.dict( "app.v2.inbound_sms.get_inbound_sms.current_app.config",