From 5b405d41aa68776e4b85ef82defdb09d5e06a3c7 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 2 Oct 2024 12:07:42 -0700 Subject: [PATCH 01/17] break test so we can see coverage numbers --- tests/app/test_commands.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/app/test_commands.py b/tests/app/test_commands.py index 46dd2b0c1..8375aa7a9 100644 --- a/tests/app/test_commands.py +++ b/tests/app/test_commands.py @@ -437,7 +437,8 @@ def test_download_csv_file_by_name(notify_api, mocker): "NonExistentName", ], ) - mock_download.assert_called_once() + mock_download.assert_not_called() + # mock_download.assert_called_once() def test_promote_user_to_platform_admin_no_result_found( From a0c27975a57ff40a31dfdea37165c008d787f1d4 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 2 Oct 2024 12:40:31 -0700 Subject: [PATCH 02/17] break test so we can see coverage numbers --- tests/app/test_commands.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/app/test_commands.py b/tests/app/test_commands.py index 8375aa7a9..46dd2b0c1 100644 --- a/tests/app/test_commands.py +++ b/tests/app/test_commands.py @@ -437,8 +437,7 @@ def test_download_csv_file_by_name(notify_api, mocker): "NonExistentName", ], ) - mock_download.assert_not_called() - # mock_download.assert_called_once() + mock_download.assert_called_once() def test_promote_user_to_platform_admin_no_result_found( From 5d265135d35fd1afcccca7ac64280f413d27c109 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 2 Oct 2024 13:08:34 -0700 Subject: [PATCH 03/17] write a test --- app/delivery/send_to_providers.py | 24 +++++++++++--------- tests/app/delivery/test_send_to_providers.py | 17 ++++++++++++++ 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 745b46cab..07763823f 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -98,17 +98,7 @@ def send_sms_to_provider(notification): # TODO This is temporary to test the capability of validating phone numbers # The future home of the validation is TBD - if "+" not in recipient: - recipient_lookup = f"+{recipient}" - else: - recipient_lookup = recipient - if recipient_lookup in current_app.config[ - "SIMULATED_SMS_NUMBERS" - ] and os.getenv("NOTIFY_ENVIRONMENT") in ["development", "test"]: - current_app.logger.info(hilite("#validate-phone-number fired")) - aws_pinpoint_client.validate_phone_number("01", recipient) - else: - current_app.logger.info(hilite("#validate-phone-number not fired")) + _experimentally_validate_phone_numbers(recipient) sender_numbers = get_sender_numbers(notification) if notification.reply_to_text not in sender_numbers: @@ -145,6 +135,18 @@ def send_sms_to_provider(notification): return message_id +def _experimentally_validate_phone_numbers(recipient): + if "+" not in recipient: + recipient_lookup = f"+{recipient}" + else: + recipient_lookup = recipient + if recipient_lookup in current_app.config["SIMULATED_SMS_NUMBERS"] and os.getenv( + "NOTIFY_ENVIRONMENT" + ) in ["development", "test"]: + current_app.logger.info(hilite("#validate-phone-number fired")) + aws_pinpoint_client.validate_phone_number("01", recipient) + + def _get_verify_code(notification): key = f"2facode-{notification.id}".replace(" ", "") recipient = redis_store.get(key) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index fbea9a2f7..4c0c39890 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -3,6 +3,7 @@ from contextlib import suppress from urllib import parse +import pytest from cachetools import TTLCache, cached from flask import current_app @@ -19,6 +20,7 @@ from app.dao.notifications_dao import dao_update_notification from app.dao.provider_details_dao import get_provider_details_by_notification_type from app.dao.service_sms_sender_dao import dao_get_sms_senders_by_service_id +from app.delivery.send_to_providers import _experimentally_validate_phone_numbers from app.enums import BrandType, KeyType, NotificationStatus, NotificationType from app.exceptions import NotificationTechnicalFailureException from app.serialised_models import SerialisedService, SerialisedTemplate @@ -306,3 +308,18 @@ def technical_failure(notification): f"Send {notification.notification_type} for notification id {notification.id} " f"to provider is not allowed: service {notification.service_id} is inactive" ) + + +@pytest.mark.parametrize( + ("recipient", "expected_invoke"), + [ + ("15555555555", False), + ], +) +def test_experimentally_validate_phone_numbers(recipient, expected_invoke, mocker): + mock_pinpoint = mocker.patch("app.delivery.send_to_providers.aws_pinpoint_client") + _experimentally_validate_phone_numbers(recipient) + if expected_invoke: + mock_pinpoint.phone_number_validate.assert_called_once_with("foo") + else: + mock_pinpoint.phone_number_validate.assert_not_called() From 6e73e81201c4b01f0d3147f5c454d052a0acfb6b Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 2 Oct 2024 13:40:19 -0700 Subject: [PATCH 04/17] ugh fix tests --- tests/app/delivery/test_send_to_providers.py | 1238 ++++++++++++++---- 1 file changed, 961 insertions(+), 277 deletions(-) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 4c0c39890..20b0f7186 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -1,313 +1,997 @@ import json -import os -from contextlib import suppress -from urllib import parse +from collections import namedtuple +from unittest.mock import ANY import pytest -from cachetools import TTLCache, cached from flask import current_app - -from app import ( - aws_pinpoint_client, - create_uuid, - db, - notification_provider_clients, - redis_store, +from requests import HTTPError + +import app +from app import aws_sns_client, notification_provider_clients +from app.cloudfoundry_config import cloud_config +from app.dao import notifications_dao +from app.dao.provider_details_dao import get_provider_details_by_identifier +from app.delivery import send_to_providers +from app.delivery.send_to_providers import ( + _experimentally_validate_phone_numbers, + get_html_email_options, + get_logo_url, ) -from app.aws.s3 import get_personalisation_from_s3, get_phone_number_from_s3 -from app.celery.test_key_tasks import send_email_response, send_sms_response -from app.dao.email_branding_dao import dao_get_email_branding_by_id -from app.dao.notifications_dao import dao_update_notification -from app.dao.provider_details_dao import get_provider_details_by_notification_type -from app.dao.service_sms_sender_dao import dao_get_sms_senders_by_service_id -from app.delivery.send_to_providers import _experimentally_validate_phone_numbers from app.enums import BrandType, KeyType, NotificationStatus, NotificationType from app.exceptions import NotificationTechnicalFailureException -from app.serialised_models import SerialisedService, SerialisedTemplate -from app.utils import hilite, utc_now -from notifications_utils.template import ( - HTMLEmailTemplate, - PlainTextEmailTemplate, - SMSMessageTemplate, +from app.models import EmailBranding, Notification +from app.serialised_models import SerialisedService +from app.utils import utc_now +from tests.app.db import ( + create_email_branding, + create_notification, + create_reply_to_email, + create_service, + create_service_sms_sender, + create_service_with_defined_sms_sender, + create_template, ) -def send_sms_to_provider(notification): - """Final step in the message send flow. - - Get data for recipient, template, - notification and send it to sns. - """ - # we no longer store the personalisation in the db, - # need to retrieve from s3 before generating content - # However, we are still sending the initial verify code through personalisation - # so if there is some value there, don't overwrite it - if not notification.personalisation: - personalisation = get_personalisation_from_s3( - notification.service_id, - notification.job_id, - notification.job_row_number, - ) - notification.personalisation = personalisation - - service = SerialisedService.from_id(notification.service_id) - message_id = None - if not service.active: - technical_failure(notification=notification) - return - - if notification.status == NotificationStatus.CREATED: - # We get the provider here (which is only aws sns) - provider = provider_to_use(NotificationType.SMS, notification.international) - if not provider: - technical_failure(notification=notification) - return - - template_model = SerialisedTemplate.from_id_and_service_id( - template_id=notification.template_id, - service_id=service.id, - version=notification.template_version, - ) - - template = SMSMessageTemplate( - template_model.__dict__, - values=notification.personalisation, - prefix=service.name, - show_prefix=service.prefix_sms, - ) - if notification.key_type == KeyType.TEST: - update_notification_to_sending(notification, provider) - send_sms_response(provider.name, str(notification.id)) - - else: - try: - # End DB session here so that we don't have a connection stuck open waiting on the call - # to one of the SMS providers - # We don't want to tie our DB connections being open to the performance of our SMS - # providers as a slow down of our providers can cause us to run out of DB connections - # Therefore we pull all the data from our DB models into `send_sms_kwargs`now before - # closing the session (as otherwise it would be reopened immediately) - - # We start by trying to get the phone number from a job in s3. If we fail, we assume - # the phone number is for the verification code on login, which is not a job. - recipient = None - # It is our 2facode, maybe - recipient = _get_verify_code(notification) - - if recipient is None: - recipient = get_phone_number_from_s3( - notification.service_id, - notification.job_id, - notification.job_row_number, - ) - - # TODO This is temporary to test the capability of validating phone numbers - # The future home of the validation is TBD - if "+" not in recipient: - recipient_lookup = f"+{recipient}" - else: - recipient_lookup = recipient - if recipient_lookup in current_app.config[ - "SIMULATED_SMS_NUMBERS" - ] and os.getenv("NOTIFY_ENVIRONMENT") in ["development", "test"]: - current_app.logger.info(hilite("#validate-phone-number fired")) - aws_pinpoint_client.validate_phone_number("01", recipient) - else: - current_app.logger.info(hilite("#validate-phone-number not fired")) - - sender_numbers = get_sender_numbers(notification) - if notification.reply_to_text not in sender_numbers: - raise ValueError( - f"{notification.reply_to_text} not in {sender_numbers} #notify-admin-1701" - ) - - send_sms_kwargs = { - "to": recipient, - "content": str(template), - "reference": str(notification.id), - "sender": notification.reply_to_text, - "international": notification.international, - } - db.session.close() # no commit needed as no changes to objects have been made above - - message_id = provider.send_sms(**send_sms_kwargs) - current_app.logger.info(f"got message_id {message_id}") - except Exception as e: - n = notification - msg = f"FAILED send to sms, job_id: {n.job_id} row_number {n.job_row_number} message_id {message_id}" - current_app.logger.exception(hilite(msg)) - - notification.billable_units = template.fragment_count - dao_update_notification(notification) - raise e - else: - # Here we map the job_id and row number to the aws message_id - n = notification - msg = f"Send to aws for job_id {n.job_id} row_number {n.job_row_number} message_id {message_id}" - current_app.logger.info(hilite(msg)) - notification.billable_units = template.fragment_count - update_notification_to_sending(notification, provider) - return message_id - - -def _get_verify_code(notification): - key = f"2facode-{notification.id}".replace(" ", "") - recipient = redis_store.get(key) - with suppress(AttributeError): - recipient = recipient.decode("utf-8") - return recipient - - -def get_sender_numbers(notification): - possible_senders = dao_get_sms_senders_by_service_id(notification.service_id) - sender_numbers = [] - for possible_sender in possible_senders: - sender_numbers.append(possible_sender.sms_sender) - return sender_numbers - - -def send_email_to_provider(notification): - # Someone needs an email, possibly new registration - recipient = redis_store.get(f"email-address-{notification.id}") - recipient = recipient.decode("utf-8") - personalisation = redis_store.get(f"email-personalisation-{notification.id}") - if personalisation: - personalisation = personalisation.decode("utf-8") - notification.personalisation = json.loads(personalisation) - - service = SerialisedService.from_id(notification.service_id) - if not service.active: - technical_failure(notification=notification) - return - if notification.status == NotificationStatus.CREATED: - provider = provider_to_use(NotificationType.EMAIL, False) - template_dict = SerialisedTemplate.from_id_and_service_id( - template_id=notification.template_id, - service_id=service.id, - version=notification.template_version, - ).__dict__ - - html_email = HTMLEmailTemplate( - template_dict, - values=notification.personalisation, - **get_html_email_options(service), - ) - - plain_text_email = PlainTextEmailTemplate( - template_dict, values=notification.personalisation - ) - - if notification.key_type == KeyType.TEST: - notification.reference = str(create_uuid()) - update_notification_to_sending(notification, provider) - send_email_response(notification.reference, recipient) - else: - from_address = ( - f'"{service.name}" <{service.email_from}@' - f'{current_app.config["NOTIFY_EMAIL_DOMAIN"]}>' - ) - - reference = provider.send_email( - from_address, - recipient, - plain_text_email.subject, - body=str(plain_text_email), - html_body=str(html_email), - reply_to_address=notification.reply_to_text, - ) - notification.reference = reference - update_notification_to_sending(notification, provider) - - -def update_notification_to_sending(notification, provider): - notification.sent_at = utc_now() - notification.sent_by = provider.name - if notification.status not in NotificationStatus.completed_types(): - notification.status = NotificationStatus.SENDING - - dao_update_notification(notification) - - -provider_cache = TTLCache(maxsize=8, ttl=10) - - -@cached(cache=provider_cache) -def provider_to_use(notification_type, international=True): - active_providers = [ - p - for p in get_provider_details_by_notification_type( - notification_type, international - ) - if p.active - ] +def setup_function(_function): + # pytest will run this function before each test. It makes sure the + # state of the cache is not shared between tests. + send_to_providers.provider_cache.clear() + + +@pytest.mark.parametrize( + "international_provider_priority", + ( + # Since there’s only one international provider it should always + # be used, no matter what its priority is set to + 0, + 50, + 100, + ), +) +def test_provider_to_use_should_only_return_sns_for_international( + mocker, + notify_db_session, + international_provider_priority, +): + sns = get_provider_details_by_identifier("sns") + sns.priority = international_provider_priority + + ret = send_to_providers.provider_to_use(NotificationType.SMS, international=True) + + assert ret.name == "sns" + + +def test_provider_to_use_raises_if_no_active_providers( + mocker, restore_provider_details +): + sns = get_provider_details_by_identifier("sns") + sns.active = False + + # flake8 doesn't like raises with a generic exception + try: + send_to_providers.provider_to_use(NotificationType.SMS) + assert 1 == 0 + except Exception: + assert 1 == 1 + + +def test_should_send_personalised_template_to_correct_sms_provider_and_persist( + sample_sms_template_with_html, mocker +): + + mocker.patch("app.delivery.send_to_providers._get_verify_code", return_value=None) + db_notification = create_notification( + template=sample_sms_template_with_html, + personalisation={}, + status=NotificationStatus.CREATED, + reply_to_text=sample_sms_template_with_html.service.get_default_sms_sender(), + ) + + mocker.patch("app.aws_sns_client.send_sms") + + mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") + mock_s3.return_value = "2028675309" + + mock_personalisation = mocker.patch( + "app.delivery.send_to_providers.get_personalisation_from_s3" + ) + mock_personalisation.return_value = {"name": "Jo"} + + send_to_providers.send_sms_to_provider(db_notification) + + aws_sns_client.send_sms.assert_called_once_with( + to="2028675309", + content="Sample service: Hello Jo\nHere is some HTML & entities", + reference=str(db_notification.id), + sender=current_app.config["FROM_NUMBER"], + international=False, + ) + + notification = Notification.query.filter_by(id=db_notification.id).one() + + assert notification.status == NotificationStatus.SENDING + assert notification.sent_at <= utc_now() + assert notification.sent_by == "sns" + assert notification.billable_units == 1 + assert notification.personalisation == {"name": "Jo"} + + +def test_should_send_personalised_template_to_correct_email_provider_and_persist( + sample_email_template_with_html, mocker +): + + mock_redis = mocker.patch("app.delivery.send_to_providers.redis_store") + utf8_encoded_email = "jo.smith@example.com".encode("utf-8") + mock_redis.get.return_value = utf8_encoded_email + email = utf8_encoded_email + personalisation = { + "name": "Jo", + } + personalisation = json.dumps(personalisation) + personalisation = personalisation.encode("utf-8") + mock_redis.get.side_effect = [email, personalisation] + db_notification = create_notification( + template=sample_email_template_with_html, + ) + db_notification.personalisation = {"name": "Jo"} + mocker.patch("app.aws_ses_client.send_email", return_value="reference") + send_to_providers.send_email_to_provider(db_notification) + app.aws_ses_client.send_email.assert_called_once_with( + f'"Sample service" ', + "jo.smith@example.com", + "Jo some HTML", + body="Hello Jo\nThis is an email from GOV.\u200bUK with some HTML\n", + html_body=ANY, + reply_to_address=None, + ) + + assert " version_on_notification + + send_to_providers.send_sms_to_provider(db_notification) + + aws_sns_client.send_sms.assert_called_once_with( + to="2028675309", + content="Sample service: This is a template:\nwith a newline", + reference=str(db_notification.id), + sender=current_app.config["FROM_NUMBER"], + international=False, + ) + + t = dao_get_template_by_id(expected_template_id) + + persisted_notification = notifications_dao.get_notification_by_id( + db_notification.id + ) + assert persisted_notification.to == db_notification.to + assert persisted_notification.template_id == expected_template_id + assert persisted_notification.template_version == version_on_notification + assert persisted_notification.template_version != t.version + assert persisted_notification.status == NotificationStatus.SENDING + + +def test_should_have_sending_status_if_fake_callback_function_fails( + sample_notification, mocker +): + mocker.patch( + "app.delivery.send_to_providers.send_sms_response", + side_effect=HTTPError, + ) + + mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") + mock_s3.return_value = "2028675309" + + mock_personalisation = mocker.patch( + "app.delivery.send_to_providers.get_personalisation_from_s3" + ) + mock_personalisation.return_value = {"ignore": "ignore"} + + sample_notification.key_type = KeyType.TEST + with pytest.raises(HTTPError): + send_to_providers.send_sms_to_provider(sample_notification) + assert sample_notification.status == NotificationStatus.SENDING + assert sample_notification.sent_by == "sns" + - if not active_providers: - current_app.logger.error(f"{notification_type} failed as no active providers") - raise Exception(f"No active {notification_type} providers") +def test_should_not_send_to_provider_when_status_is_not_created( + sample_template, mocker +): + notification = create_notification( + template=sample_template, + status=NotificationStatus.SENDING, + ) + mocker.patch("app.aws_sns_client.send_sms") + response_mock = mocker.patch("app.delivery.send_to_providers.send_sms_response") + + mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") + mock_s3.return_value = "2028675309" + + mock_personalisation = mocker.patch( + "app.delivery.send_to_providers.get_personalisation_from_s3" + ) + mock_personalisation.return_value = {"ignore": "ignore"} + + send_to_providers.send_sms_to_provider(notification) + + app.aws_sns_client.send_sms.assert_not_called() + response_mock.assert_not_called() + + +def test_should_send_sms_with_downgraded_content(notify_db_session, mocker): + # é, o, and u are in GSM. + # ī, grapes, tabs, zero width space and ellipsis are not + # ó isn't in GSM, but it is in the welsh alphabet so will still be sent - # we only have sns - chosen_provider = active_providers[0] + mocker.patch("app.delivery.send_to_providers.redis_store", return_value=None) + mocker.patch( + "app.delivery.send_to_providers.get_sender_numbers", return_value=["testing"] + ) + msg = "a é ī o u 🍇 foo\tbar\u200bbaz((misc))…" + placeholder = "∆∆∆abc" + gsm_message = "?ódz Housing Service: a é i o u ? foo barbaz???abc..." + service = create_service(service_name="Łódź Housing Service") + template = create_template(service, content=msg) + db_notification = create_notification( + template=template, + ) + db_notification.personalisation = {"misc": placeholder} + db_notification.reply_to_text = "testing" + + mocker.patch("app.aws_sns_client.send_sms") + + mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") + mock_phone.return_value = "15555555555" - return notification_provider_clients.get_client_by_name_and_type( - chosen_provider.identifier, notification_type + mock_personalisation = mocker.patch( + "app.delivery.send_to_providers.get_personalisation_from_s3" ) + mock_personalisation.return_value = {"misc": placeholder} + send_to_providers.send_sms_to_provider(db_notification) + + aws_sns_client.send_sms.assert_called_once_with( + to=ANY, content=gsm_message, reference=ANY, sender=ANY, international=False + ) -def get_logo_url(base_url, logo_file): - base_url = parse.urlparse(base_url) - netloc = base_url.netloc - if base_url.netloc.startswith("localhost"): - netloc = "notify.tools" - elif base_url.netloc.startswith("www"): - # strip "www." - netloc = base_url.netloc[4:] +def test_send_sms_should_use_service_sms_sender( + sample_service, sample_template, mocker +): - logo_url = parse.ParseResult( - scheme=base_url.scheme, - netloc="static-logos." + netloc, - path=logo_file, - params=base_url.params, - query=base_url.query, - fragment=base_url.fragment, + mocker.patch("app.delivery.send_to_providers.redis_store", return_value=None) + mocker.patch("app.aws_sns_client.send_sms") + + sms_sender = create_service_sms_sender( + service=sample_service, sms_sender="123456", is_default=False + ) + db_notification = create_notification( + template=sample_template, reply_to_text=sms_sender.sms_sender ) - return parse.urlunparse(logo_url) + expected_sender_name = sms_sender.sms_sender + mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") + mock_phone.return_value = "15555555555" + + mock_personalisation = mocker.patch( + "app.delivery.send_to_providers.get_personalisation_from_s3" + ) + mock_personalisation.return_value = {"ignore": "ignore"} + + send_to_providers.send_sms_to_provider( + db_notification, + ) + + app.aws_sns_client.send_sms.assert_called_once_with( + to=ANY, + content=ANY, + reference=ANY, + sender=expected_sender_name, + international=False, + ) + + +def test_send_email_to_provider_should_not_send_to_provider_when_status_is_not_created( + sample_email_template, mocker +): + mock_redis = mocker.patch("app.delivery.send_to_providers.redis_store") + mock_redis.get.return_value = "test@example.com".encode("utf-8") + + notification = create_notification( + template=sample_email_template, status=NotificationStatus.SENDING + ) + mocker.patch("app.aws_ses_client.send_email") + mocker.patch("app.delivery.send_to_providers.send_email_response") + mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") + mock_phone.return_value = "15555555555" + + mock_personalisation = mocker.patch( + "app.delivery.send_to_providers.get_personalisation_from_s3" + ) + mock_personalisation.return_value = {"ignore": "ignore"} + send_to_providers.send_sms_to_provider(notification) + app.aws_ses_client.send_email.assert_not_called() + app.delivery.send_to_providers.send_email_response.assert_not_called() + + +def test_send_email_should_use_service_reply_to_email( + sample_service, sample_email_template, mocker +): + mocker.patch("app.aws_ses_client.send_email", return_value="reference") + + mock_redis = mocker.patch("app.delivery.send_to_providers.redis_store") + mock_redis.get.return_value = "test@example.com".encode("utf-8") + mock_redis = mocker.patch("app.delivery.send_to_providers.redis_store") + email = "foo@bar.com".encode("utf-8") + personalisation = {} -def get_html_email_options(service): - if service.email_branding is None: - return { - "govuk_banner": True, - "brand_banner": False, - } - if isinstance(service, SerialisedService): - branding = dao_get_email_branding_by_id(service.email_branding) + personalisation = json.dumps(personalisation) + personalisation = personalisation.encode("utf-8") + mock_redis.get.side_effect = [email, personalisation] + + db_notification = create_notification( + template=sample_email_template, reply_to_text="foo@bar.com" + ) + create_reply_to_email(service=sample_service, email_address="foo@bar.com") + + send_to_providers.send_email_to_provider(db_notification) + + app.aws_ses_client.send_email.assert_called_once_with( + ANY, + ANY, + ANY, + body=ANY, + html_body=ANY, + reply_to_address="foo@bar.com", + ) + + +def test_get_html_email_renderer_should_return_for_normal_service(sample_service): + options = send_to_providers.get_html_email_options(sample_service) + assert options["govuk_banner"] is True + assert "brand_colour" not in options.keys() + assert "brand_logo" not in options.keys() + assert "brand_text" not in options.keys() + assert "brand_name" not in options.keys() + + +@pytest.mark.parametrize( + "branding_type, govuk_banner", + [(BrandType.ORG, False), (BrandType.BOTH, True), (BrandType.ORG_BANNER, False)], +) +def test_get_html_email_renderer_with_branding_details( + branding_type, govuk_banner, notify_db_session, sample_service +): + email_branding = EmailBranding( + brand_type=branding_type, + colour="#000000", + logo="justice-league.png", + name="Justice League", + text="League of Justice", + ) + sample_service.email_branding = email_branding + notify_db_session.add_all([sample_service, email_branding]) + notify_db_session.commit() + + options = send_to_providers.get_html_email_options(sample_service) + + assert options["govuk_banner"] == govuk_banner + assert options["brand_colour"] == "#000000" + assert options["brand_text"] == "League of Justice" + assert options["brand_name"] == "Justice League" + + if branding_type == BrandType.ORG_BANNER: + assert options["brand_banner"] is True else: - branding = service.email_branding + assert options["brand_banner"] is False + + +def test_get_html_email_renderer_with_branding_details_and_render_govuk_banner_only( + notify_db_session, sample_service +): + sample_service.email_branding = None + notify_db_session.add_all([sample_service]) + notify_db_session.commit() + + options = send_to_providers.get_html_email_options(sample_service) + + assert options == {"govuk_banner": True, "brand_banner": False} + + +def test_get_html_email_renderer_prepends_logo_path(notify_api): + Service = namedtuple("Service", ["email_branding"]) + EmailBranding = namedtuple( + "EmailBranding", + ["brand_type", "colour", "name", "logo", "text"], + ) + + email_branding = EmailBranding( + brand_type=BrandType.ORG, + colour="#000000", + logo="justice-league.png", + name="Justice League", + text="League of Justice", + ) + service = Service( + email_branding=email_branding, + ) + + renderer = send_to_providers.get_html_email_options(service) + + assert ( + renderer["brand_logo"] == "http://static-logos.notify.tools/justice-league.png" + ) + + +def test_get_html_email_renderer_handles_email_branding_without_logo(notify_api): + Service = namedtuple("Service", ["email_branding"]) + EmailBranding = namedtuple( + "EmailBranding", + ["brand_type", "colour", "name", "logo", "text"], + ) + + email_branding = EmailBranding( + brand_type=BrandType.ORG_BANNER, + colour="#000000", + logo=None, + name="Justice League", + text="League of Justice", + ) + service = Service( + email_branding=email_branding, + ) + + renderer = send_to_providers.get_html_email_options(service) + + assert renderer["govuk_banner"] is False + assert renderer["brand_banner"] is True + assert renderer["brand_logo"] is None + assert renderer["brand_text"] == "League of Justice" + assert renderer["brand_colour"] == "#000000" + assert renderer["brand_name"] == "Justice League" + + +@pytest.mark.parametrize( + "base_url, expected_url", + [ + # don't change localhost to prevent errors when testing locally + ("http://localhost:6012", "http://static-logos.notify.tools/filename.png"), + ( + "https://www.notifications.service.gov.uk", + "https://static-logos.notifications.service.gov.uk/filename.png", + ), + ("https://notify.works", "https://static-logos.notify.works/filename.png"), + ( + "https://staging-notify.works", + "https://static-logos.staging-notify.works/filename.png", + ), + ("https://www.notify.works", "https://static-logos.notify.works/filename.png"), + ( + "https://www.staging-notify.works", + "https://static-logos.staging-notify.works/filename.png", + ), + ], +) +def test_get_logo_url_works_for_different_environments(base_url, expected_url): + logo_file = "filename.png" + + logo_url = send_to_providers.get_logo_url(base_url, logo_file) + + assert logo_url == expected_url + + +@pytest.mark.parametrize( + "starting_status, expected_status", + [ + (NotificationStatus.DELIVERED, NotificationStatus.DELIVERED), + (NotificationStatus.CREATED, NotificationStatus.SENDING), + (NotificationStatus.TECHNICAL_FAILURE, NotificationStatus.TECHNICAL_FAILURE), + ], +) +def test_update_notification_to_sending_does_not_update_status_from_a_final_status( + sample_service, notify_db_session, starting_status, expected_status +): + template = create_template(sample_service) + notification = create_notification(template=template, status=starting_status) + send_to_providers.update_notification_to_sending( + notification, + notification_provider_clients.get_client_by_name_and_type( + "sns", NotificationType.SMS + ), + ) + assert notification.status == expected_status + + +def __update_notification(notification_to_update, research_mode, expected_status): + if research_mode or notification_to_update.key_type == KeyType.TEST: + notification_to_update.status = expected_status + + +@pytest.mark.parametrize( + "research_mode,key_type, billable_units, expected_status", + [ + (True, KeyType.NORMAL, 0, NotificationStatus.DELIVERED), + (False, KeyType.NORMAL, 1, NotificationStatus.SENDING), + (False, KeyType.TEST, 0, NotificationStatus.SENDING), + (True, KeyType.TEST, 0, NotificationStatus.SENDING), + (True, KeyType.TEAM, 0, NotificationStatus.DELIVERED), + (False, KeyType.TEAM, 1, NotificationStatus.SENDING), + ], +) +def test_should_update_billable_units_and_status_according_to_research_mode_and_key_type( + sample_template, mocker, research_mode, key_type, billable_units, expected_status +): + + mocker.patch("app.delivery.send_to_providers.redis_store", return_value=None) + mocker.patch( + "app.delivery.send_to_providers.get_sender_numbers", return_value=["testing"] + ) + notification = create_notification( + template=sample_template, + billable_units=0, + status=NotificationStatus.CREATED, + key_type=key_type, + reply_to_text="testing", + ) + mocker.patch("app.aws_sns_client.send_sms") + mocker.patch( + "app.delivery.send_to_providers.send_sms_response", + side_effect=__update_notification(notification, research_mode, expected_status), + ) + + if research_mode: + sample_template.service.research_mode = True + + mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") + mock_phone.return_value = "15555555555" + + mock_personalisation = mocker.patch( + "app.delivery.send_to_providers.get_personalisation_from_s3" + ) + # So we don't treat it as a one off and have to mock other things + mock_personalisation.return_value = {"ignore": "ignore"} + + send_to_providers.send_sms_to_provider(notification) + assert notification.billable_units == billable_units + assert notification.status == expected_status + + +def test_should_set_notification_billable_units_and_reduces_provider_priority_if_sending_to_provider_fails( + sample_notification, + mocker, +): + mocker.patch("app.aws_sns_client.send_sms", side_effect=Exception()) + + sample_notification.billable_units = 0 + assert sample_notification.sent_by is None + + mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") + mock_phone.return_value = "15555555555" + + mock_personalisation = mocker.patch( + "app.delivery.send_to_providers.get_personalisation_from_s3" + ) + mock_personalisation.return_value = {"ignore": "ignore"} + + # flake8 no longer likes raises with a generic exception + try: + send_to_providers.send_sms_to_provider(sample_notification) + assert 1 == 0 + except Exception: + assert 1 == 1 + + assert sample_notification.billable_units == 1 + + +def test_should_send_sms_to_international_providers( + sample_template, sample_user, mocker +): + + mocker.patch("app.delivery.send_to_providers._get_verify_code", return_value=None) + mocker.patch("app.aws_sns_client.send_sms") + + notification_international = create_notification( + template=sample_template, + to_field="+6011-17224412", + personalisation={"name": "Jo"}, + status=NotificationStatus.CREATED, + international=True, + reply_to_text=sample_template.service.get_default_sms_sender(), + normalised_to="601117224412", + ) + + mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") + mock_s3.return_value = "601117224412" + + mock_personalisation = mocker.patch( + "app.delivery.send_to_providers.get_personalisation_from_s3" + ) + mock_personalisation.return_value = {"ignore": "ignore"} + + send_to_providers.send_sms_to_provider(notification_international) + + aws_sns_client.send_sms.assert_called_once_with( + to="601117224412", + content=ANY, + reference=str(notification_international.id), + sender=current_app.config["FROM_NUMBER"], + international=True, + ) + + assert notification_international.status == NotificationStatus.SENDING + assert notification_international.sent_by == "sns" + + +@pytest.mark.parametrize( + "sms_sender, expected_sender, prefix_sms, expected_content", + [ + ("foo", "foo", False, "bar"), + ("foo", "foo", True, "Sample service: bar"), + # if 40604 is actually in DB then treat that as if entered manually + ("40604", "40604", False, "bar"), + # 'testing' is the FROM_NUMBER during unit tests + ("testing", "testing", True, "Sample service: bar"), + ("testing", "testing", False, "bar"), + ], +) +def test_should_handle_sms_sender_and_prefix_message( + mocker, sms_sender, prefix_sms, expected_sender, expected_content, notify_db_session +): + + mocker.patch("app.delivery.send_to_providers.redis_store", return_value=None) + mocker.patch("app.aws_sns_client.send_sms") + service = create_service_with_defined_sms_sender( + sms_sender_value=sms_sender, prefix_sms=prefix_sms + ) + template = create_template(service, content="bar") + notification = create_notification(template, reply_to_text=sms_sender) + + mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") + mock_phone.return_value = "15555555555" + + mock_personalisation = mocker.patch( + "app.delivery.send_to_providers.get_personalisation_from_s3" + ) + mock_personalisation.return_value = {"ignore": "ignore"} + + send_to_providers.send_sms_to_provider(notification) - logo_url = ( - get_logo_url(current_app.config["ADMIN_BASE_URL"], branding.logo) - if branding.logo - else None + aws_sns_client.send_sms.assert_called_once_with( + content=expected_content, + sender=expected_sender, + to=ANY, + reference=ANY, + international=False, ) - return { + +def test_send_email_to_provider_uses_reply_to_from_notification( + sample_email_template, mocker +): + mock_redis = mocker.patch("app.delivery.send_to_providers.redis_store") + mock_redis.get.side_effect = [ + "test@example.com".encode("utf-8"), + json.dumps({}).encode("utf-8"), + ] + + mocker.patch("app.aws_ses_client.send_email", return_value="reference") + + db_notification = create_notification( + template=sample_email_template, + reply_to_text="test@test.com", + ) + + send_to_providers.send_email_to_provider(db_notification) + + app.aws_ses_client.send_email.assert_called_once_with( + ANY, + ANY, + ANY, + body=ANY, + html_body=ANY, + reply_to_address="test@test.com", + ) + + +def test_send_sms_to_provider_should_use_normalised_to(mocker, client, sample_template): + + mocker.patch("app.delivery.send_to_providers._get_verify_code", return_value=None) + mocker.patch( + "app.delivery.send_to_providers.get_sender_numbers", return_value=["testing"] + ) + send_mock = mocker.patch("app.aws_sns_client.send_sms") + notification = create_notification( + template=sample_template, + to_field="+12028675309", + normalised_to="2028675309", + reply_to_text="testing", + ) + + mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") + mock_s3.return_value = "12028675309" + + mock_personalisation = mocker.patch( + "app.delivery.send_to_providers.get_personalisation_from_s3" + ) + mock_personalisation.return_value = {"ignore": "ignore"} + send_to_providers.send_sms_to_provider(notification) + send_mock.assert_called_once_with( + to="12028675309", + content=ANY, + reference=str(notification.id), + sender=notification.reply_to_text, + international=False, + ) + + +def test_send_email_to_provider_should_user_normalised_to( + mocker, client, sample_email_template +): + send_mock = mocker.patch("app.aws_ses_client.send_email", return_value="reference") + notification = create_notification( + template=sample_email_template, + ) + mock_redis = mocker.patch("app.delivery.send_to_providers.redis_store") + mock_redis.get.return_value = "test@example.com".encode("utf-8") + + mock_redis = mocker.patch("app.delivery.send_to_providers.redis_store") + mock_redis.get.return_value = "jo.smith@example.com".encode("utf-8") + email = "test@example.com".encode("utf-8") + personalisation = {} + + personalisation = json.dumps(personalisation) + personalisation = personalisation.encode("utf-8") + mock_redis.get.side_effect = [email, personalisation] + + send_to_providers.send_email_to_provider(notification) + send_mock.assert_called_once_with( + ANY, + "test@example.com", + ANY, + body=ANY, + html_body=ANY, + reply_to_address=notification.reply_to_text, + ) + + +def test_send_sms_to_provider_should_return_template_if_found_in_redis( + mocker, client, sample_template +): + + mocker.patch("app.delivery.send_to_providers._get_verify_code", return_value=None) + mocker.patch( + "app.delivery.send_to_providers.get_sender_numbers", return_value=["testing"] + ) + from app.schemas import service_schema, template_schema + + service_dict = service_schema.dump(sample_template.service) + template_dict = template_schema.dump(sample_template) + + mocker.patch( + "app.redis_store.get", + side_effect=[ + json.dumps({"data": service_dict}).encode("utf-8"), + json.dumps({"data": template_dict}).encode("utf-8"), + ], + ) + mock_get_template = mocker.patch( + "app.dao.templates_dao.dao_get_template_by_id_and_service_id" + ) + mock_get_service = mocker.patch("app.dao.services_dao.dao_fetch_service_by_id") + + send_mock = mocker.patch("app.aws_sns_client.send_sms") + notification = create_notification( + template=sample_template, + to_field="+447700900855", + normalised_to="447700900855", + reply_to_text="testing", + ) + + mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") + mock_s3.return_value = "447700900855" + + mock_personalisation = mocker.patch( + "app.delivery.send_to_providers.get_personalisation_from_s3" + ) + mock_personalisation.return_value = {"ignore": "ignore"} + + send_to_providers.send_sms_to_provider(notification) + assert mock_get_template.called is False + assert mock_get_service.called is False + send_mock.assert_called_once_with( + to="447700900855", + content=ANY, + reference=str(notification.id), + sender=notification.reply_to_text, + international=False, + ) + + +def test_send_email_to_provider_should_return_template_if_found_in_redis( + mocker, client, sample_email_template +): + from app.schemas import service_schema, template_schema + + # mock_redis = mocker.patch("app.delivery.send_to_providers.redis_store") + # mock_redis.get.return_value = "jo.smith@example.com".encode("utf-8") + email = "test@example.com".encode("utf-8") + personalisation = { + "name": "Jo", + } + + personalisation = json.dumps(personalisation) + personalisation = personalisation.encode("utf-8") + # mock_redis.get.side_effect = [email, personalisation] + + service_dict = service_schema.dump(sample_email_template.service) + template_dict = template_schema.dump(sample_email_template) + + mocker.patch( + "app.redis_store.get", + side_effect=[ + email, + personalisation, + json.dumps({"data": service_dict}).encode("utf-8"), + json.dumps({"data": template_dict}).encode("utf-8"), + ], + ) + mock_get_template = mocker.patch( + "app.dao.templates_dao.dao_get_template_by_id_and_service_id" + ) + mock_get_service = mocker.patch("app.dao.services_dao.dao_fetch_service_by_id") + send_mock = mocker.patch("app.aws_ses_client.send_email", return_value="reference") + notification = create_notification( + template=sample_email_template, + ) + + send_to_providers.send_email_to_provider(notification) + assert mock_get_template.called is False + assert mock_get_service.called is False + send_mock.assert_called_once_with( + ANY, + "test@example.com", + ANY, + body=ANY, + html_body=ANY, + reply_to_address=notification.reply_to_text, + ) + + +def test_get_html_email_options_return_email_branding_from_serialised_service( + sample_service, +): + branding = create_email_branding() + sample_service.email_branding = branding + service = SerialisedService.from_id(sample_service.id) + email_options = get_html_email_options(service) + assert email_options is not None + assert email_options == { "govuk_banner": branding.brand_type == BrandType.BOTH, "brand_banner": branding.brand_type == BrandType.ORG_BANNER, "brand_colour": branding.colour, - "brand_logo": logo_url, + "brand_logo": get_logo_url(current_app.config["ADMIN_BASE_URL"], branding.logo), "brand_text": branding.text, "brand_name": branding.name, } -def technical_failure(notification): - notification.status = NotificationStatus.TECHNICAL_FAILURE - dao_update_notification(notification) - raise NotificationTechnicalFailureException( - f"Send {notification.notification_type} for notification id {notification.id} " - f"to provider is not allowed: service {notification.service_id} is inactive" - ) +def test_get_html_email_options_add_email_branding_from_service(sample_service): + branding = create_email_branding() + sample_service.email_branding = branding + email_options = get_html_email_options(sample_service) + assert email_options is not None + assert email_options == { + "govuk_banner": branding.brand_type == BrandType.BOTH, + "brand_banner": branding.brand_type == BrandType.ORG_BANNER, + "brand_colour": branding.colour, + "brand_logo": get_logo_url(current_app.config["ADMIN_BASE_URL"], branding.logo), + "brand_text": branding.text, + "brand_name": branding.name, + } @pytest.mark.parametrize( From 05a6a2a4d9f69f52fa40cb5eea43684730bd180a Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 2 Oct 2024 14:04:26 -0700 Subject: [PATCH 05/17] comment out strange command we may never use --- app/commands.py | 181 ++++++++++++++++++++++++------------------------ 1 file changed, 89 insertions(+), 92 deletions(-) diff --git a/app/commands.py b/app/commands.py index 45fce9211..1c761f84a 100644 --- a/app/commands.py +++ b/app/commands.py @@ -24,12 +24,6 @@ dao_create_or_update_annual_billing_for_year, set_default_free_allowance_for_service, ) -from app.dao.fact_billing_dao import ( - delete_billing_data_for_service_for_day, - fetch_billing_data_for_day, - get_service_ids_that_need_billing_populated, - update_fact_billing, -) from app.dao.jobs_dao import dao_get_job_by_id from app.dao.organization_dao import ( dao_add_service_to_organization, @@ -63,7 +57,7 @@ TemplateHistory, User, ) -from app.utils import get_midnight_in_utc, utc_now +from app.utils import utc_now from notifications_utils.recipients import RecipientCSV from notifications_utils.template import SMSMessageTemplate from tests.app.db import ( @@ -167,76 +161,78 @@ def purge_functional_test_data(user_email_prefix): delete_model_user(usr) -@notify_command(name="insert-inbound-numbers") -@click.option( - "-f", - "--file_name", - required=True, - help="""Full path of the file to upload, file is a contains inbound numbers, one number per line.""", -) -def insert_inbound_numbers_from_file(file_name): - # TODO maintainability what is the purpose of this command? Who would use it and why? +# TODO maintainability what is the purpose of this command? Who would use it and why? +# COMMENTING OUT UNTIL WE DETERMINE IF WE NEED IT OR NOT +# @notify_command(name="insert-inbound-numbers") +# @click.option( +# "-f", +# "--file_name", +# required=True, +# help="""Full path of the file to upload, file is a contains inbound numbers, one number per line.""", +# ) +# def insert_inbound_numbers_from_file(file_name): - current_app.logger.info(f"Inserting inbound numbers from {file_name}") - with open(file_name) as file: - sql = text( - "insert into inbound_numbers values(:uuid, :line, 'sns', null, True, now(), null);" - ) +# current_app.logger.info(f"Inserting inbound numbers from {file_name}") +# with open(file_name) as file: +# sql = text( +# "insert into inbound_numbers values(:uuid, :line, 'sns', null, True, now(), null);" +# ) - for line in file: - line = line.strip() - if line: - current_app.logger.info(line) - db.session.execute(sql, {"uuid": str(uuid.uuid4()), "line": line}) - db.session.commit() +# for line in file: +# line = line.strip() +# if line: +# current_app.logger.info(line) +# db.session.execute(sql, {"uuid": str(uuid.uuid4()), "line": line}) +# db.session.commit() def setup_commands(application): application.cli.add_command(command_group) -@notify_command(name="rebuild-ft-billing-for-day") -@click.option("-s", "--service_id", required=False, type=click.UUID) -@click.option( - "-d", - "--day", - help="The date to recalculate, as YYYY-MM-DD", - required=True, - type=click_dt(format="%Y-%m-%d"), -) -def rebuild_ft_billing_for_day(service_id, day): - # TODO maintainability what is the purpose of this command? Who would use it and why? - - """ - Rebuild the data in ft_billing for the given service_id and date - """ - - def rebuild_ft_data(process_day, service): - deleted_rows = delete_billing_data_for_service_for_day(process_day, service) - current_app.logger.info( - f"deleted {deleted_rows} existing billing rows for {service} on {process_day}" - ) - transit_data = fetch_billing_data_for_day( - process_day=process_day, service_id=service - ) - # transit_data = every row that should exist - for data in transit_data: - # upsert existing rows - update_fact_billing(data, process_day) - current_app.logger.info( - f"added/updated {len(transit_data)} billing rows for {service} on {process_day}" - ) - - if service_id: - # confirm the service exists - dao_fetch_service_by_id(service_id) - rebuild_ft_data(day, service_id) - else: - services = get_service_ids_that_need_billing_populated( - get_midnight_in_utc(day), get_midnight_in_utc(day + timedelta(days=1)) - ) - for row in services: - rebuild_ft_data(day, row.service_id) +# TODO maintainability what is the purpose of this command? Who would use it and why? +# COMMENTING OUT UNTIL WE DETERMINE IF WE NEED IT OR NOT +# @notify_command(name="rebuild-ft-billing-for-day") +# @click.option("-s", "--service_id", required=False, type=click.UUID) +# @click.option( +# "-d", +# "--day", +# help="The date to recalculate, as YYYY-MM-DD", +# required=True, +# type=click_dt(format="%Y-%m-%d"), +# ) +# def rebuild_ft_billing_for_day(service_id, day): + +# """ +# Rebuild the data in ft_billing for the given service_id and date +# """ + +# def rebuild_ft_data(process_day, service): +# deleted_rows = delete_billing_data_for_service_for_day(process_day, service) +# current_app.logger.info( +# f"deleted {deleted_rows} existing billing rows for {service} on {process_day}" +# ) +# transit_data = fetch_billing_data_for_day( +# process_day=process_day, service_id=service +# ) +# # transit_data = every row that should exist +# for data in transit_data: +# # upsert existing rows +# update_fact_billing(data, process_day) +# current_app.logger.info( +# f"added/updated {len(transit_data)} billing rows for {service} on {process_day}" +# ) + +# if service_id: +# # confirm the service exists +# dao_fetch_service_by_id(service_id) +# rebuild_ft_data(day, service_id) +# else: +# services = get_service_ids_that_need_billing_populated( +# get_midnight_in_utc(day), get_midnight_in_utc(day + timedelta(days=1)) +# ) +# for row in services: +# rebuild_ft_data(day, row.service_id) @notify_command(name="bulk-invite-user-to-service") @@ -472,29 +468,30 @@ def associate_services_to_organizations(): current_app.logger.info("finished associating services to organizations") -@notify_command(name="populate-service-volume-intentions") -@click.option( - "-f", - "--file_name", - required=True, - help="Pipe delimited file containing service_id, SMS, email", -) -def populate_service_volume_intentions(file_name): - # [0] service_id - # [1] SMS:: volume intentions for service - # [2] Email:: volume intentions for service - - # TODO maintainability what is the purpose of this command? Who would use it and why? - - with open(file_name, "r") as f: - for line in itertools.islice(f, 1, None): - columns = line.split(",") - current_app.logger.info(columns) - service = dao_fetch_service_by_id(columns[0]) - service.volume_sms = columns[1] - service.volume_email = columns[2] - dao_update_service(service) - current_app.logger.info("populate-service-volume-intentions complete") +# TODO maintainability what is the purpose of this command? Who would use it and why? +# COMMENTING OUT UNTIL WE DETERMINE IF WE NEED IT OR NOT +# @notify_command(name="populate-service-volume-intentions") +# @click.option( +# "-f", +# "--file_name", +# required=True, +# help="Pipe delimited file containing service_id, SMS, email", +# ) +# def populate_service_volume_intentions(file_name): +# # [0] service_id +# # [1] SMS:: volume intentions for service +# # [2] Email:: volume intentions for service + + +# with open(file_name, "r") as f: +# for line in itertools.islice(f, 1, None): +# columns = line.split(",") +# current_app.logger.info(columns) +# service = dao_fetch_service_by_id(columns[0]) +# service.volume_sms = columns[1] +# service.volume_email = columns[2] +# dao_update_service(service) +# current_app.logger.info("populate-service-volume-intentions complete") @notify_command(name="populate-go-live") From 06643c3bb50dc58963fa7dae7139843aa5fdb862 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 2 Oct 2024 14:11:47 -0700 Subject: [PATCH 06/17] comment out strange command we may never use --- app/commands.py | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/app/commands.py b/app/commands.py index 1c761f84a..a43ae06ca 100644 --- a/app/commands.py +++ b/app/commands.py @@ -162,28 +162,27 @@ def purge_functional_test_data(user_email_prefix): # TODO maintainability what is the purpose of this command? Who would use it and why? -# COMMENTING OUT UNTIL WE DETERMINE IF WE NEED IT OR NOT -# @notify_command(name="insert-inbound-numbers") -# @click.option( -# "-f", -# "--file_name", -# required=True, -# help="""Full path of the file to upload, file is a contains inbound numbers, one number per line.""", -# ) -# def insert_inbound_numbers_from_file(file_name): +@notify_command(name="insert-inbound-numbers") +@click.option( + "-f", + "--file_name", + required=True, + help="""Full path of the file to upload, file is a contains inbound numbers, one number per line.""", +) +def insert_inbound_numbers_from_file(file_name): -# current_app.logger.info(f"Inserting inbound numbers from {file_name}") -# with open(file_name) as file: -# sql = text( -# "insert into inbound_numbers values(:uuid, :line, 'sns', null, True, now(), null);" -# ) + current_app.logger.info(f"Inserting inbound numbers from {file_name}") + with open(file_name) as file: + sql = text( + "insert into inbound_numbers values(:uuid, :line, 'sns', null, True, now(), null);" + ) -# for line in file: -# line = line.strip() -# if line: -# current_app.logger.info(line) -# db.session.execute(sql, {"uuid": str(uuid.uuid4()), "line": line}) -# db.session.commit() + for line in file: + line = line.strip() + if line: + current_app.logger.info(line) + db.session.execute(sql, {"uuid": str(uuid.uuid4()), "line": line}) + db.session.commit() def setup_commands(application): From face881a90ecb0ab40f3ee535080e8bd71bb6b1c Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 2 Oct 2024 14:45:22 -0700 Subject: [PATCH 07/17] clean up sanitise_text --- notifications_utils/sanitise_text.py | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/notifications_utils/sanitise_text.py b/notifications_utils/sanitise_text.py index 3e9da0764..5a1d1c382 100644 --- a/notifications_utils/sanitise_text.py +++ b/notifications_utils/sanitise_text.py @@ -122,19 +122,15 @@ def is_arabic(cls, value): def is_punjabi(cls, value): # Gukmukhi script or Shahmukhi script - if regex.search(r"[\u0A00-\u0A7F]+", value): - return True - elif regex.search(r"[\u0600-\u06FF]+", value): - return True - elif regex.search(r"[\u0750-\u077F]+", value): - return True - elif regex.search(r"[\u08A0-\u08FF]+", value): - return True - elif regex.search(r"[\uFB50-\uFDFF]+", value): - return True - elif regex.search(r"[\uFE70-\uFEFF]+", value): - return True - elif regex.search(r"[\u0900-\u097F]+", value): + if ( + regex.search(r"[\u0A00-\u0A7F]+", value) + or regex.search(r"[\u0600-\u06FF]+", value) + or regex.search(r"[\u0750-\u077F]+", value) + or regex.search(r"[\u08A0-\u08FF]+", value) + or regex.search(r"[\uFB50-\uFDFF]+", value) + or regex.search(r"[\uFE70-\uFEFF]+", value) + or regex.search(r"[\u0900-\u097F]+", value) + ): return True return False From dba29a8ea7fff2d5f651c3669462dcc8bc3e9115 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 2 Oct 2024 14:55:15 -0700 Subject: [PATCH 08/17] clean up sanitise_text --- notifications_utils/sanitise_text.py | 38 ++++++++++++---------------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/notifications_utils/sanitise_text.py b/notifications_utils/sanitise_text.py index 5a1d1c382..750a2e49b 100644 --- a/notifications_utils/sanitise_text.py +++ b/notifications_utils/sanitise_text.py @@ -152,33 +152,27 @@ def _is_extended_language_group_one(cls, value): @classmethod def _is_extended_language_group_two(cls, value): - if regex.search(r"\p{IsBuhid}", value): - return True - if regex.search(r"\p{IsCanadian_Aboriginal}", value): - return True - if regex.search(r"\p{IsCherokee}", value): - return True - if regex.search(r"\p{IsDevanagari}", value): - return True - if regex.search(r"\p{IsEthiopic}", value): - return True - if regex.search(r"\p{IsGeorgian}", value): + if ( + regex.search(r"\p{IsBuhid}", value) + or regex.search(r"\p{IsCanadian_Aboriginal}", value) + or regex.search(r"\p{IsCherokee}", value) + or regex.search(r"\p{IsDevanagari}", value) + or regex.search(r"\p{IsEthiopic}", value) + or regex.search(r"\p{IsGeorgian}", value) + ): return True return False @classmethod def _is_extended_language_group_three(cls, value): - if regex.search(r"\p{IsGreek}", value): - return True - if regex.search(r"\p{IsGujarati}", value): - return True - if regex.search(r"\p{IsHanunoo}", value): - return True - if regex.search(r"\p{IsHebrew}", value): - return True - if regex.search(r"\p{IsLimbu}", value): - return True - if regex.search(r"\p{IsKannada}", value): + if ( + regex.search(r"\p{IsGreek}", value) + or regex.search(r"\p{IsGujarati}", value) + or regex.search(r"\p{IsHanunoo}", value) + or regex.search(r"\p{IsHebrew}", value) + or regex.search(r"\p{IsLimbu}", value) + or regex.search(r"\p{IsKannada}", value) + ): return True return False From 445a462b1052353e85475b284fb93c69db71650f Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 2 Oct 2024 15:00:03 -0700 Subject: [PATCH 09/17] clean up s3 --- app/aws/s3.py | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index bd0301d78..dc293ea6f 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -466,23 +466,7 @@ def get_personalisation_from_s3(service_id, job_id, job_row_number): set_job_cache(job_cache, f"{job_id}_personalisation", extract_personalisation(job)) - # If we can find the quick dictionary, use it - if job_cache.get(f"{job_id}_personalisation") is not None: - personalisation_to_return = job_cache.get(f"{job_id}_personalisation")[0].get( - job_row_number - ) - if personalisation_to_return: - return personalisation_to_return - else: - current_app.logger.warning( - f"Was unable to retrieve personalisation from lookup dictionary for job {job_id}" - ) - return {} - else: - current_app.logger.error( - f"Was unable to construct lookup dictionary for job {job_id}" - ) - return {} + return job_cache.get(f"{job_id}_personalisation")[0].get(job_row_number) def get_job_metadata_from_s3(service_id, job_id): From 76373de13b9f05b4b71c37d5206125af480c066e Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 3 Oct 2024 08:54:16 -0700 Subject: [PATCH 10/17] comment out search for notification by to field --- app/service/rest.py | 110 ++++++++++++++++++++++---------------------- 1 file changed, 55 insertions(+), 55 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index 070f13457..9ae507adb 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -453,16 +453,16 @@ def get_all_notifications_for_service(service_id): data = notifications_filter_schema.load(MultiDict(request.get_json())) current_app.logger.debug(f"use POST, request {request.get_json()} data {data}") - if data.get("to"): - notification_type = ( - data.get("template_type")[0] if data.get("template_type") else None - ) - return search_for_notification_by_to_field( - service_id=service_id, - search_term=data["to"], - statuses=data.get("status"), - notification_type=notification_type, - ) + # if data.get("to"): + # notification_type = ( + # data.get("template_type")[0] if data.get("template_type") else None + # ) + # return search_for_notification_by_to_field( + # service_id=service_id, + # search_term=data["to"], + # statuses=data.get("status"), + # notification_type=notification_type, + # ) page = data["page"] if "page" in data else 1 page_size = ( data["page_size"] @@ -583,51 +583,51 @@ def get_notification_for_service(service_id, notification_id): ) -def search_for_notification_by_to_field( - service_id, search_term, statuses, notification_type -): - results = notifications_dao.dao_get_notifications_by_recipient_or_reference( - service_id=service_id, - search_term=search_term, - statuses=statuses, - notification_type=notification_type, - page=1, - page_size=current_app.config["PAGE_SIZE"], - ) - - # We try and get the next page of results to work out if we need provide a pagination link to the next page - # in our response. Note, this was previously be done by having - # notifications_dao.dao_get_notifications_by_recipient_or_reference use count=False when calling - # Flask-Sqlalchemys `paginate'. But instead we now use this way because it is much more performant for - # services with many results (unlike using Flask SqlAlchemy `paginate` with `count=True`, this approach - # doesn't do an additional query to count all the results of which there could be millions but instead only - # asks for a single extra page of results). - next_page_of_pagination = notifications_dao.dao_get_notifications_by_recipient_or_reference( - service_id=service_id, - search_term=search_term, - statuses=statuses, - notification_type=notification_type, - page=2, - page_size=current_app.config["PAGE_SIZE"], - error_out=False, # False so that if there are no results, it doesn't end in aborting with a 404 - ) - - return ( - jsonify( - notifications=notification_with_template_schema.dump( - results.items, many=True - ), - links=get_prev_next_pagination_links( - 1, - len(next_page_of_pagination.items), - ".get_all_notifications_for_service", - statuses=statuses, - notification_type=notification_type, - service_id=service_id, - ), - ), - 200, - ) +# def search_for_notification_by_to_field( +# service_id, search_term, statuses, notification_type +# ): +# results = notifications_dao.dao_get_notifications_by_recipient_or_reference( +# service_id=service_id, +# search_term=search_term, +# statuses=statuses, +# notification_type=notification_type, +# page=1, +# page_size=current_app.config["PAGE_SIZE"], +# ) + +# # We try and get the next page of results to work out if we need provide a pagination link to the next page +# # in our response. Note, this was previously be done by having +# # notifications_dao.dao_get_notifications_by_recipient_or_reference use count=False when calling +# # Flask-Sqlalchemys `paginate'. But instead we now use this way because it is much more performant for +# # services with many results (unlike using Flask SqlAlchemy `paginate` with `count=True`, this approach +# # doesn't do an additional query to count all the results of which there could be millions but instead only +# # asks for a single extra page of results). +# next_page_of_pagination = notifications_dao.dao_get_notifications_by_recipient_or_reference( +# service_id=service_id, +# search_term=search_term, +# statuses=statuses, +# notification_type=notification_type, +# page=2, +# page_size=current_app.config["PAGE_SIZE"], +# error_out=False, # False so that if there are no results, it doesn't end in aborting with a 404 +# ) + +# return ( +# jsonify( +# notifications=notification_with_template_schema.dump( +# results.items, many=True +# ), +# links=get_prev_next_pagination_links( +# 1, +# len(next_page_of_pagination.items), +# ".get_all_notifications_for_service", +# statuses=statuses, +# notification_type=notification_type, +# service_id=service_id, +# ), +# ), +# 200, +# ) @service_blueprint.route("//notifications/monthly", methods=["GET"]) From 38583c28eaa99ba081a58baa196de5e01fc8ae15 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 3 Oct 2024 09:23:16 -0700 Subject: [PATCH 11/17] add a test in service rest --- tests/app/service/test_rest.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index fec71cf82..5e179b708 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1959,6 +1959,29 @@ def test_get_all_notifications_for_service_including_ones_made_by_jobs( assert response.status_code == 200 +def test_get_monthly_notification_stats_by_user( + client, + sample_service, + sample_user, + mocker, +): + mock_s3 = mocker.patch("app.service.rest.get_phone_number_from_s3") + mock_s3.return_value = "" + + mock_s3 = mocker.patch("app.service.rest.get_personalisation_from_s3") + mock_s3.return_value = {} + + auth_header = create_admin_authorization_header() + + response = client.get( + path=(f"/service/{sample_service.id}/notifications/{sample_user.id}/monthly"), + headers=[auth_header], + ) + + # TODO This test could be a little more complete + assert response.status_code == 200 + + def test_get_only_api_created_notifications_for_service( admin_request, sample_job, From 55966267c2d13bd861885c3bf1b33717a19bced1 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 3 Oct 2024 09:35:05 -0700 Subject: [PATCH 12/17] add a test in service rest --- tests/app/service/test_rest.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 5e179b708..d6f87d0f6 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1975,9 +1975,12 @@ def test_get_monthly_notification_stats_by_user( response = client.get( path=(f"/service/{sample_service.id}/notifications/{sample_user.id}/monthly"), + year=2024, headers=[auth_header], ) + resp = json.loads(response.get_data(as_text=True)) + print(f"RESP is {resp}") # TODO This test could be a little more complete assert response.status_code == 200 From b0735ffcdce7808e2be10bf3ddda2dcc0798871a Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 3 Oct 2024 09:48:32 -0700 Subject: [PATCH 13/17] add a test in service rest --- tests/app/service/test_rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index d6f87d0f6..1c5170596 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1975,8 +1975,8 @@ def test_get_monthly_notification_stats_by_user( response = client.get( path=(f"/service/{sample_service.id}/notifications/{sample_user.id}/monthly"), - year=2024, headers=[auth_header], + year="2024", ) resp = json.loads(response.get_data(as_text=True)) From 5277f7066035b6f7e0c444f94d3083f8b2b820b1 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 3 Oct 2024 09:59:41 -0700 Subject: [PATCH 14/17] add a test in service rest --- tests/app/service/test_rest.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 1c5170596..d17f778de 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1974,9 +1974,8 @@ def test_get_monthly_notification_stats_by_user( auth_header = create_admin_authorization_header() response = client.get( - path=(f"/service/{sample_service.id}/notifications/{sample_user.id}/monthly"), + path=(f"/service/{sample_service.id}/notifications/{sample_user.id}/monthly?year=2024"), headers=[auth_header], - year="2024", ) resp = json.loads(response.get_data(as_text=True)) From 6d05c1a18ba39308bbe7d4290959608c42565f39 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 3 Oct 2024 10:13:37 -0700 Subject: [PATCH 15/17] add a test in service rest --- tests/app/service/test_rest.py | 49 ++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index d17f778de..70104930f 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1984,6 +1984,55 @@ def test_get_monthly_notification_stats_by_user( assert response.status_code == 200 +def test_get_single_month_notification_stats_by_user( + client, + sample_service, + sample_user, + mocker, +): + mock_s3 = mocker.patch("app.service.rest.get_phone_number_from_s3") + mock_s3.return_value = "" + + mock_s3 = mocker.patch("app.service.rest.get_personalisation_from_s3") + mock_s3.return_value = {} + + auth_header = create_admin_authorization_header() + + response = client.get( + path=(f"/service/{sample_service.id}/notifications/{sample_user.id}/month/?year=2024&month=07"), + headers=[auth_header], + ) + + resp = json.loads(response.get_data(as_text=True)) + print(f"RESP is {resp}") + # TODO This test could be a little more complete + assert response.status_code == 200 + + +def test_get_single_month_notification_stats_for_service( + client, + sample_service, + mocker, +): + mock_s3 = mocker.patch("app.service.rest.get_phone_number_from_s3") + mock_s3.return_value = "" + + mock_s3 = mocker.patch("app.service.rest.get_personalisation_from_s3") + mock_s3.return_value = {} + + auth_header = create_admin_authorization_header() + + response = client.get( + path=(f"/service/{sample_service.id}/notifications/month/?year=2024&month=07"), + headers=[auth_header], + ) + + resp = json.loads(response.get_data(as_text=True)) + print(f"RESP is {resp}") + # TODO This test could be a little more complete + assert response.status_code == 200 + + def test_get_only_api_created_notifications_for_service( admin_request, sample_job, From 1c95cd63e76afbb0f2b80c42014a9b778ac84b41 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 3 Oct 2024 10:22:32 -0700 Subject: [PATCH 16/17] add a test in service rest --- tests/app/service/test_rest.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 70104930f..5ea6e1168 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1999,7 +1999,7 @@ def test_get_single_month_notification_stats_by_user( auth_header = create_admin_authorization_header() response = client.get( - path=(f"/service/{sample_service.id}/notifications/{sample_user.id}/month/?year=2024&month=07"), + path=(f"/service/{sample_service.id}/notifications/{sample_user.id}/month?year=2024&month=07"), headers=[auth_header], ) @@ -2023,7 +2023,7 @@ def test_get_single_month_notification_stats_for_service( auth_header = create_admin_authorization_header() response = client.get( - path=(f"/service/{sample_service.id}/notifications/month/?year=2024&month=07"), + path=(f"/service/{sample_service.id}/notifications/month?year=2024&month=07"), headers=[auth_header], ) From ff5d405a1528f3027e0de6854fff52528b974a80 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 3 Oct 2024 10:30:30 -0700 Subject: [PATCH 17/17] raise coverage to 93 --- .github/workflows/checks.yml | 2 +- Makefile | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 22c7f9c89..bcf0861e4 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -63,7 +63,7 @@ jobs: NOTIFY_E2E_TEST_PASSWORD: ${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} - name: Check coverage threshold # TODO get this back up to 95 - run: poetry run coverage report -m --fail-under=91 + run: poetry run coverage report -m --fail-under=93 validate-new-relic-config: runs-on: ubuntu-latest diff --git a/Makefile b/Makefile index 88cf6f814..76c38d94e 100644 --- a/Makefile +++ b/Makefile @@ -84,7 +84,7 @@ test: ## Run tests and create coverage report poetry run coverage run --omit=*/migrations/*,*/tests/* -m pytest --maxfail=10 ## TODO set this back to 95 asap - poetry run coverage report -m --fail-under=91 + poetry run coverage report -m --fail-under=93 poetry run coverage html -d .coverage_cache .PHONY: py-lock