Pushed the cache increment into the shared code that persists notifications.

Much simpler implementation, inc code removed from tasks and V1/V2 rest clients.
2025-12-15 09:42:38 -05:00 · 2016-11-22 12:53:20 +00:00
parent 58bbc5a5aa
commit 9e2ba9ee81
9 changed files with 52 additions and 167 deletions
--- a/app/celery/tasks.py
+++ b/app/celery/tasks.py
@@ -28,7 +28,7 @@ from app.notifications.process_notifications import persist_notification
 from app.service.utils import service_allowed_to_send_to
 from app.statsd_decorators import statsd
 from app import redis_store
-from app.clients.redis import cache_key
+from app.clients.redis import daily_limit_cache_key


@notify_celery.task(name="process-job")
@@ -164,8 +164,6 @@ def send_sms(self,
                "RETRY FAILED: task send_sms failed for notification".format(
                    notification.get('job', None),
                    notification.get('row_number', None)), e)
-    else:
-        redis_store.incr(cache_key(service_id))


@notify_celery.task(bind=True, name="send-email", max_retries=5, default_retry_delay=300)
@@ -216,5 +214,3 @@ def send_email(self, service_id,
                "RETRY FAILED: task send_email failed for notification".format(
                    notification.get('job', None),
                    notification.get('row_number', None)), e)
-        else:
-            redis_store.incr(cache_key(service_id))
--- a/app/clients/redis/init.py
+++ b/app/clients/redis/init.py
@@ -1,5 +1,5 @@
 from datetime import datetime


-def cache_key(service_id):
+def daily_limit_cache_key(service_id):
    return "{}-{}-{}".format(str(service_id), datetime.utcnow().strftime("%Y-%m-%d"), "count")
--- a/app/notifications/process_notifications.py
+++ b/app/notifications/process_notifications.py
@@ -4,8 +4,9 @@ from flask import current_app
 from notifications_utils.renderers import PassThrough
 from notifications_utils.template import Template

-from app import DATETIME_FORMAT
+from app import DATETIME_FORMAT, redis_store
 from app.celery import provider_tasks
+from app.clients import redis
 from app.dao.notifications_dao import dao_create_notification, dao_delete_notifications_and_history_by_id
 from app.models import SMS_TYPE, Notification, KEY_TYPE_TEST, EMAIL_TYPE
 from app.notifications.validators import check_sms_content_char_count
@@ -61,6 +62,7 @@ def persist_notification(template_id,
        job_row_number=job_row_number
    )
    dao_create_notification(notification)
+    redis_store.incr(redis.daily_limit_cache_key(service_id))
    return notification


--- a/app/notifications/rest.py
+++ b/app/notifications/rest.py
@@ -246,7 +246,6 @@ def send_notification(notification_type):

    notification_id = create_uuid() if saved_notification is None else saved_notification.id
    notification.update({"template_version": template.version})
-    redis_store.incr(redis.cache_key(service.id))
    return jsonify(
        data=get_notification_return_data(
            notification_id,
--- a/app/notifications/validators.py
+++ b/app/notifications/validators.py
@@ -10,7 +10,7 @@ from app.clients import redis

 def check_service_message_limit(key_type, service):
    if key_type != KEY_TYPE_TEST:
-        cache_key = redis.cache_key(service.id)
+        cache_key = redis.daily_limit_cache_key(service.id)
        service_stats = redis_store.get(cache_key)
        if not service_stats:
            service_stats = services_dao.fetch_todays_total_message_count(service.id)
--- a/tests/app/celery/test_tasks.py
+++ b/tests/app/celery/test_tasks.py
@@ -332,7 +332,6 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi
                                          to="+447234123123", personalisation={"name": "Jo"})

        mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
-        redis_mock = mocker.patch('app.celery.tasks.redis_store.incr')

        send_sms(
            sample_template_with_placeholders.service_id,
@@ -357,7 +356,6 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi
            [str(persisted_notification.id)],
            queue="send-sms"
        )
-        redis_mock.assert_called_with(str(sample_template_with_placeholders.service_id) + '-2016-01-01-count')


 def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_service(notify_db, notify_db_session, mocker):
@@ -513,48 +511,6 @@ def test_should_not_send_email_if_restricted_service_and_invalid_email_address(n
    assert Notification.query.count() == 0


-def test_should_not_not_increment_counter_if_not_sending_sms(notify_db, notify_db_session, mocker):
-    redis_mock = mocker.patch('app.celery.tasks.redis_store.incr')
-
-    user = sample_user(notify_db, notify_db_session)
-    service = sample_service(notify_db, notify_db_session, user=user, restricted=True)
-    template = sample_template(
-        notify_db, notify_db_session, service=service, template_type='sms'
-    )
-    notification = _notification_json(template, to="+447878787878")
-
-    notification_id = uuid.uuid4()
-    send_sms(
-        service.id,
-        notification_id,
-        encryption.encrypt(notification),
-        datetime.utcnow().strftime(DATETIME_FORMAT)
-    )
-
-    redis_mock.assert_not_called()
-
-
-def test_should_not_not_increment_counter_if_not_sending_email(notify_db, notify_db_session, mocker):
-    redis_mock = mocker.patch('app.celery.tasks.redis_store.incr')
-
-    user = sample_user(notify_db, notify_db_session)
-    service = sample_service(notify_db, notify_db_session, user=user, restricted=True)
-    template = sample_template(
-        notify_db, notify_db_session, service=service, template_type='email', subject_line='Hello'
-    )
-    notification = _notification_json(template, to="test@example.com")
-
-    notification_id = uuid.uuid4()
-    send_email(
-        service.id,
-        notification_id,
-        encryption.encrypt(notification),
-        datetime.utcnow().strftime(DATETIME_FORMAT)
-    )
-
-    redis_mock.assert_not_called()
-
-
 def test_should_put_send_email_task_in_research_mode_queue_if_research_mode_service(
        notify_db, notify_db_session, mocker
 ):
@@ -683,7 +639,6 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh
            {"name": "Jo"},
            row_number=1)
        mocker.patch('app.celery.provider_tasks.deliver_email.apply_async')
-        redis_mock = mocker.patch('app.celery.tasks.redis_store.incr')

        notification_id = uuid.uuid4()

--- a/tests/app/clients/test_redis_client.py
+++ b/tests/app/clients/test_redis_client.py
@@ -2,7 +2,7 @@ import pytest
 from unittest.mock import Mock

 from app.clients.redis.redis_client import RedisClient
-from app.clients.redis import cache_key
+from app.clients.redis import daily_limit_cache_key
 from freezegun import freeze_time


@@ -78,4 +78,4 @@ def test_should_call_get_if_enabled(enabled_redis_client):

 def test_should_build_cache_key_service_and_action(sample_service):
    with freeze_time("2016-01-01 12:00:00.000000"):
-        assert cache_key(sample_service.id) == '{}-2016-01-01-count'.format(sample_service.id)
+        assert daily_limit_cache_key(sample_service.id) == '{}-2016-01-01-count'.format(sample_service.id)
--- a/tests/app/notifications/rest/test_send_notification.py
+++ b/tests/app/notifications/rest/test_send_notification.py
@@ -27,115 +27,6 @@ from app.models import Template
 from app.errors import InvalidRequest


-@freeze_time("2016-01-01 11:09:00.061258")
-def test_should_increment_redis_cache_on_successful_email(
-        notify_api,
-        sample_email_template,
-        mocker):
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
-            mocked_email = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async')
-            mocked_redis = mocker.patch('app.notifications.rest.redis_store.incr')
-            mocker.patch('app.encryption.encrypt', return_value="something_encrypted")
-
-            data = {
-                'to': 'ok@ok.com',
-                'template': str(sample_email_template.id)
-            }
-            auth_header = create_authorization_header(service_id=sample_email_template.service_id)
-
-            response = client.post(
-                path='/notifications/email',
-                data=json.dumps(data),
-                headers=[('Content-Type', 'application/json'), auth_header])
-
-            response_data = json.loads(response.data)['data']
-            notification_id = response_data['notification']['id']
-
-            mocked_email.assert_called_once_with([notification_id], queue='send-email')
-            mocked_redis.assert_called_once_with(str(sample_email_template.service_id) + "-2016-01-01-count")
-            assert response.status_code == 201
-            assert notification_id
-            assert response_data['subject'] == 'Email Subject'
-            assert response_data['body'] == sample_email_template.content
-            assert response_data['template_version'] == sample_email_template.version
-
-
-@freeze_time("2016-01-01 11:09:00.061258")
-def test_should_increment_redis_cache_on_successful_sms(
-        notify_api,
-        sample_template,
-        mocker):
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
-            mocked_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
-            mocked_redis = mocker.patch('app.notifications.rest.redis_store.incr')
-            mocker.patch('app.encryption.encrypt', return_value="something_encrypted")
-
-            data = {
-                'to': '+447700900855',
-                'template': str(sample_template.id)
-            }
-            auth_header = create_authorization_header(service_id=sample_template.service_id)
-
-            response = client.post(
-                path='/notifications/sms',
-                data=json.dumps(data),
-                headers=[('Content-Type', 'application/json'), auth_header])
-
-            response_data = json.loads(response.data)['data']
-            notification_id = response_data['notification']['id']
-
-            mocked_sms.assert_called_once_with([notification_id], queue='send-sms')
-            mocked_redis.assert_called_once_with(str(sample_template.service_id) + "-2016-01-01-count")
-            assert response.status_code == 201
-            assert notification_id
-            assert 'subject' not in response_data
-            assert response_data['body'] == sample_template.content
-            assert response_data['template_version'] == sample_template.version
-
-
-@pytest.mark.parametrize('template_type', ['sms', 'email'])
-def test_should_not_increment_cache_on_failure(
-        notify_api,
-        sample_email_template,
-        sample_template,
-        fake_uuid,
-        mocker,
-        template_type):
-    with notify_api.test_request_context(), notify_api.test_client() as client:
-        mocked = mocker.patch(
-            'app.celery.provider_tasks.deliver_{}.apply_async'.format(template_type),
-            side_effect=Exception("failed to talk to SQS")
-        )
-        mocked_redis = mocker.patch('app.notifications.rest.redis_store.incr')
-        mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid)
-        template = sample_template if template_type == 'sms' else sample_email_template
-        to = sample_template.service.created_by.mobile_number if template_type == 'sms' \
-            else sample_email_template.service.created_by.email_address
-        data = {
-            'to': to,
-            'template': template.id
-        }
-        api_key = ApiKey(
-            service=template.service,
-            name='team_key',
-            created_by=template.created_by,
-            key_type=KEY_TYPE_TEAM)
-        save_model_api_key(api_key)
-        auth_header = create_jwt_token(secret=api_key.unsigned_secret, client_id=str(api_key.service_id))
-
-        response = client.post(
-            path='/notifications/{}'.format(template_type),
-            data=json.dumps(data),
-            headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))])
-
-        mocked.assert_called_once_with([fake_uuid], queue='send-{}'.format(template_type))
-
-        assert response.status_code == 500
-        mocked_redis.assert_not_called()
-
-
@pytest.mark.parametrize('template_type',
                         ['sms', 'email'])
 def test_create_notification_should_reject_if_missing_required_fields(notify_api,
--- a/tests/app/notifications/test_process_notification.py
+++ b/tests/app/notifications/test_process_notification.py
@@ -3,6 +3,7 @@ import datetime
 import pytest
 from boto3.exceptions import Boto3Error
 from sqlalchemy.exc import SQLAlchemyError
+from freezegun import freeze_time

 from app.models import Template, Notification, NotificationHistory
 from app.notifications import SendNotificationToQueueError
@@ -38,7 +39,10 @@ def test_create_content_for_notification_fails_with_additional_personalisation(s
    assert e.value.message == 'Template personalisation not needed for template: Additional placeholder'


-def test_persist_notification_creates_and_save_to_db(sample_template, sample_api_key):
+@freeze_time("2016-01-01 11:09:00.061258")
+def test_persist_notification_creates_and_save_to_db(sample_template, sample_api_key, mocker):
+    mocked_redis = mocker.patch('app.notifications.process_notifications.redis_store.incr')
+
    assert Notification.query.count() == 0
    assert NotificationHistory.query.count() == 0
    notification = persist_notification(sample_template.id, sample_template.version, '+447111111111',
@@ -47,6 +51,7 @@ def test_persist_notification_creates_and_save_to_db(sample_template, sample_api
    assert Notification.query.count() == 1
    assert Notification.query.get(notification.id) is not None
    assert NotificationHistory.query.count() == 1
+    mocked_redis.assert_called_once_with(str(sample_template.service_id) + "-2016-01-01-count")


 def test_persist_notification_throws_exception_when_missing_template(sample_api_key):
@@ -65,9 +70,45 @@ def test_persist_notification_throws_exception_when_missing_template(sample_api_
    assert NotificationHistory.query.count() == 0


-def test_persist_notification_with_job_and_created(sample_job, sample_api_key):
+def test_exception_thown_by_redis_store_get_should_not_be_fatal(sample_template, sample_api_key, mocker):
+    mocker.patch(
+        'app.notifications.process_notifications.redis_store.redis_store.incr',
+        side_effect=Exception("broken redis"))
+
+    notification = persist_notification(
+        sample_template.id,
+        sample_template.version,
+        '+447111111111',
+        sample_template.service.id,
+        {},
+        'sms',
+        sample_api_key.id,
+        sample_api_key.key_type)
+    assert Notification.query.count() == 1
+    assert Notification.query.get(notification.id) is not None
+    assert NotificationHistory.query.count() == 1
+
+
+def test_cache_is_not_incremented_on_failure_to_persist_notification(sample_api_key, mocker):
+    mocked_redis = mocker.patch('app.notifications.process_notifications.redis_store.incr')
+    with pytest.raises(SQLAlchemyError):
+        persist_notification(template_id=None,
+                             template_version=None,
+                             recipient='+447111111111',
+                             service_id=sample_api_key.service_id,
+                             personalisation=None,
+                             notification_type='sms',
+                             api_key_id=sample_api_key.id,
+                             key_type=sample_api_key.key_type)
+    mocked_redis.assert_not_called()
+
+
+@freeze_time("2016-01-01 11:09:00.061258")
+def test_persist_notification_with_job_and_created(sample_job, sample_api_key, mocker):
    assert Notification.query.count() == 0
    assert NotificationHistory.query.count() == 0
+    mocked_redis = mocker.patch('app.notifications.process_notifications.redis_store.incr')
+
    created_at = datetime.datetime(2016, 11, 11, 16, 8, 18)
    persist_notification(template_id=sample_job.template.id,
                         template_version=sample_job.template.version,
@@ -85,6 +126,7 @@ def test_persist_notification_with_job_and_created(sample_job, sample_api_key):
    assert persisted_notification.job_id == sample_job.id
    assert persisted_notification.job_row_number == 10
    assert persisted_notification.created_at == created_at
+    mocked_redis.assert_called_once_with(str(sample_job.service_id) + "-2016-01-01-count")


@pytest.mark.parametrize('research_mode, queue, notification_type, key_type',