From e9dfbeca376f6f2e74486778482fbdc22cf8d15a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 3 Jul 2020 14:27:28 +0100 Subject: [PATCH 1/2] Bump utils to 40.2.1 Changes: https://github.com/alphagov/notifications-utils/compare/40.0.0...40.2.1 --- app/serialised_models.py | 51 ++++------------------------------------ requirements-app.txt | 2 +- requirements.txt | 6 ++--- 3 files changed, 8 insertions(+), 51 deletions(-) diff --git a/app/serialised_models.py b/app/serialised_models.py index 2b3af3903..1b17726f0 100644 --- a/app/serialised_models.py +++ b/app/serialised_models.py @@ -1,10 +1,13 @@ -from abc import ABC, abstractmethod from collections import defaultdict from functools import partial from threading import RLock import cachetools from notifications_utils.clients.redis import RequestCache +from notifications_utils.serialised_model import ( + SerialisedModel, + SerialisedModelCollection, +) from werkzeug.utils import cached_property from app import db, redis_store @@ -33,52 +36,6 @@ def ignore_first_argument_cache_key(cls, *args, **kwargs): return cachetools.keys.hashkey(*args, **kwargs) -class SerialisedModel(ABC): - - """ - A SerialisedModel takes a dictionary, typically created by - serialising a database object. It then takes the value of specified - keys from the dictionary and adds them to itself as properties, so - that it can be interacted with like a normal database model object, - but with no risk that it will actually go back to the database. - """ - - @property - @abstractmethod - def ALLOWED_PROPERTIES(self): - pass - - def __init__(self, _dict): - for property in self.ALLOWED_PROPERTIES: - setattr(self, property, _dict[property]) - - def __dir__(self): - return super().__dir__() + list(sorted(self.ALLOWED_PROPERTIES)) - - -class SerialisedModelCollection(ABC): - - """ - A SerialisedModelCollection takes a list of dictionaries, typically - created by serialising database objects. When iterated over it - returns a SerialisedModel instance for each of the items in the list. - """ - - @property - @abstractmethod - def model(self): - pass - - def __init__(self, items): - self.items = items - - def __bool__(self): - return bool(self.items) - - def __getitem__(self, index): - return self.model(self.items[index]) - - class SerialisedTemplate(SerialisedModel): ALLOWED_PROPERTIES = { 'archived', diff --git a/requirements-app.txt b/requirements-app.txt index 58add7c6c..50f3b6f28 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -27,7 +27,7 @@ notifications-python-client==5.5.1 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@40.0.0#egg=notifications-utils==40.0.0 +git+https://github.com/alphagov/notifications-utils.git@40.2.1#egg=notifications-utils==40.2.1 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.7.1 diff --git a/requirements.txt b/requirements.txt index 3d1087c14..e42c6931c 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,7 +29,7 @@ notifications-python-client==5.5.1 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@40.0.0#egg=notifications-utils==40.0.0 +git+https://github.com/alphagov/notifications-utils.git@40.2.1#egg=notifications-utils==40.2.1 # 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.89 +awscli==1.18.93 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.12 +botocore==1.17.16 certifi==2020.6.20 chardet==3.0.4 click==7.1.2 From efdaadbdf4508baa714e2e52ad18c19f3d286e36 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Thu, 2 Jul 2020 18:22:06 +0100 Subject: [PATCH 2/2] Do not update notification to sending if the status is already final This prevents a race condition when we get delivery receipt before updating notification to sending, and so the sending status would supersede the delivered status, and the notification would time out as temporary-failure after three days. --- app/delivery/send_to_providers.py | 6 ++++-- tests/app/delivery/test_send_to_providers.py | 16 +++++++++++++++- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index c50858ccb..3f92e54d1 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -28,7 +28,8 @@ from app.models import ( EMAIL_TYPE, NOTIFICATION_TECHNICAL_FAILURE, NOTIFICATION_SENT, - NOTIFICATION_SENDING + NOTIFICATION_SENDING, + NOTIFICATION_STATUS_TYPES_COMPLETED ) @@ -125,7 +126,8 @@ def send_email_to_provider(notification): def update_notification_to_sending(notification, provider): notification.sent_at = datetime.utcnow() notification.sent_by = provider.get_name() - notification.status = NOTIFICATION_SENT if notification.international else NOTIFICATION_SENDING + if notification.status not in NOTIFICATION_STATUS_TYPES_COMPLETED: + notification.status = NOTIFICATION_SENT if notification.international else NOTIFICATION_SENDING dao_update_notification(notification) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index dc19ff4ee..367f86059 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -9,7 +9,7 @@ from notifications_utils.recipients import validate_and_format_phone_number from requests import HTTPError import app -from app import mmg_client, firetext_client +from app import clients, mmg_client, firetext_client 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 @@ -525,6 +525,20 @@ def test_should_not_update_notification_if_research_mode_on_exception( assert update_mock.called +@pytest.mark.parametrize("starting_status, expected_status", [ + ("delivered", "delivered"), + ("created", "sending"), + ("technical-failure", "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, clients.get_client_by_name_and_type("mmg", "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 == KEY_TYPE_TEST: notification_to_update.status = expected_status