Merge branch 'master' into clean-up

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(

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)

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:

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(
+    created_by = field_for(models.Template, 'created_by', required=True)
-        models.Template, 'created_by_id', dump_to='created_by', dump_only=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',

app/serialised_models.py

@@ -120,7 +120,9 @@ class SerialisedService(SerialisedModel):
         'active',
         'contact_link',
         'email_from',
+        'message_limit',
         'permissions',
+        'rate_limit',
         'research_mode',
         'restricted',
     }

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

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
+idna==2.10
-importlib-metadata==1.6.1
+importlib-metadata==1.7.0
 Jinja2==2.11.2
 jmespath==0.10.0
 kombu==3.0.37

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)
+    check_service_over_daily_message_limit(key_type, serialised_service)
-    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
 
 
@@ -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)
+    serialised_service = SerialisedService.from_id(sample_service.id)
-    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
 
 
 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
 
 

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])

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 = {

tests/app/test_user_agent_processing.py

@@ -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)

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",