From 6205bbdff5ef366b243c8d13339bd77d7c64602a Mon Sep 17 00:00:00 2001 From: Jumana B Date: Wed, 26 Jul 2023 11:44:21 -0400 Subject: [PATCH] Backend: Task to institute a Daily email limit (#1937) * Validation for deciding whether to send email limit emails * Add db migrations * update notification utils * update waffles * Makefile updates * try * tests for validators * Add check for redis in jobs and fix tests * Add feature flag where daily message check is called in tasks * Add code for validators and post notifications * try * fix * poetry lock update --- .github/workflows/test.yaml | 2 +- Makefile | 4 + app/celery/tasks.py | 6 +- app/config.py | 7 + app/dao/services_dao.py | 2 +- app/email_limit_utils.py | 1 - app/job/rest.py | 22 +-- app/notifications/validators.py | 102 +++++++++++- app/service/rest.py | 4 +- app/service/send_notification.py | 6 +- app/v2/errors.py | 16 ++ app/v2/notifications/post_notifications.py | 7 + .../0432_daily_email_limit_templates.py | 154 ++++++++++++++++++ poetry.lock | 8 +- pyproject.toml | 2 +- tests/app/celery/test_tasks.py | 16 +- tests/app/job/test_rest.py | 3 + tests/app/notifications/test_validators.py | 86 +++++++--- tests/app/service/test_rest.py | 4 +- .../service/test_send_one_off_notification.py | 21 +++ 20 files changed, 417 insertions(+), 56 deletions(-) create mode 100644 migrations/versions/0432_daily_email_limit_templates.py diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 0d86a6b3e1..ebd718b8c6 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -67,7 +67,7 @@ jobs: run: | cp -f .env.example .env - name: Checks for new endpoints against AWS WAF rules - uses: cds-snc/notification-utils/.github/actions/waffles@52.0.3 + uses: cds-snc/notification-utils/.github/actions/waffles@52.0.4 with: app-loc: '/github/workspace' app-libs: '/github/workspace/env/site-packages' diff --git a/Makefile b/Makefile index 8ad12676a6..1fe12eada5 100644 --- a/Makefile +++ b/Makefile @@ -69,3 +69,7 @@ run-celery-beat: ## Run the celery beat .PHONY: run-celery-purge run-celery-purge: ## Purge the celery queues ./scripts/run_celery_purge.sh + +.PHONY: run-db +run-db: ## psql to access dev database + psql postgres://postgres:chummy@db:5432/notification_api \ No newline at end of file diff --git a/app/celery/tasks.py b/app/celery/tasks.py index e69f972144..544b932585 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -306,7 +306,8 @@ def save_smss(self, service_id: Optional[str], signed_notifications: List[Signed current_app.logger.info(f"Sending following sms notifications to AWS: {notification_id_queue.keys()}") for notification_obj in saved_notifications: try: - check_service_over_daily_message_limit(notification_obj.key_type, service) + if not current_app.config["FF_EMAIL_DAILY_LIMIT"]: + check_service_over_daily_message_limit(notification_obj.key_type, service) queue = notification_id_queue.get(notification_obj.id) or template.queue_to_use() # type: ignore send_notification_to_queue( notification_obj, @@ -433,7 +434,8 @@ def try_to_send_notifications_to_queue(notification_id_queue, service, saved_not research_mode = service.research_mode # type: ignore for notification_obj in saved_notifications: try: - check_service_over_daily_message_limit(notification_obj.key_type, service) + if not current_app.config["FF_EMAIL_DAILY_LIMIT"]: + check_service_over_daily_message_limit(notification_obj.key_type, service) queue = notification_id_queue.get(notification_obj.id) or template.queue_to_use() # type: ignore send_notification_to_queue( notification_obj, diff --git a/app/config.py b/app/config.py index b83e63f9ec..da8b718790 100644 --- a/app/config.py +++ b/app/config.py @@ -270,6 +270,9 @@ class Config(object): REACHED_DAILY_SMS_LIMIT_TEMPLATE_ID = "a646e614-c527-4f94-a955-ed7185d577f4" DAILY_SMS_LIMIT_UPDATED_TEMPLATE_ID = "6ec12dd0-680a-4073-8d58-91d17cc8442f" CONTACT_FORM_DIRECT_EMAIL_TEMPLATE_ID = "b04beb4a-8408-4280-9a5c-6a046b6f7704" + NEAR_DAILY_EMAIL_LIMIT_TEMPLATE_ID = "9aa60ad7-2d7f-46f0-8cbe-2bac3d4d77d8" + REACHED_DAILY_EMAIL_LIMIT_TEMPLATE_ID = "ee036547-e51b-49f1-862b-10ea982cfceb" + DAILY_EMAIL_LIMIT_UPDATED_TEMPLATE_ID = "97dade64-ea8d-460f-8a34-900b74ee5eb0" # Allowed service IDs able to send HTML through their templates. ALLOW_HTML_SERVICE_IDS: List[str] = [id.strip() for id in os.getenv("ALLOW_HTML_SERVICE_IDS", "").split(",")] @@ -525,6 +528,9 @@ class Config(object): # Timestamp in epoch milliseconds to seed the bounce rate. We will seed data for (24, the below config) included. FF_BOUNCE_RATE_SEED_EPOCH_MS = os.getenv("FF_BOUNCE_RATE_SEED_EPOCH_MS", False) + # Feature flags for email_daily_limit + FF_EMAIL_DAILY_LIMIT = env.bool("FF_EMAIL_DAILY_LIMIT", False) + @classmethod def get_sensitive_config(cls) -> list[str]: "List of config keys that contain sensitive information" @@ -621,6 +627,7 @@ class Test(Development): TEMPLATE_PREVIEW_API_HOST = "http://localhost:9999" FF_BOUNCE_RATE_BACKEND = True + FF_EMAIL_DAILY_LIMIT = False class Production(Config): diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index a9a73fbe7e..a7f33f2917 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -454,7 +454,7 @@ def fetch_service_email_limit(service_id: uuid.UUID) -> int: def fetch_todays_total_email_count(service_id: uuid.UUID) -> int: midnight = get_midnight(datetime.now(tz=pytz.utc)) result = ( - db.session.query(func.count(Notification).label("total_email_notifications")) + db.session.query(func.count(Notification.id).label("total_email_notifications")) .filter( Notification.service_id == service_id, Notification.key_type != KEY_TYPE_TEST, diff --git a/app/email_limit_utils.py b/app/email_limit_utils.py index e24fcada73..7086d5c6f0 100644 --- a/app/email_limit_utils.py +++ b/app/email_limit_utils.py @@ -23,7 +23,6 @@ def fetch_todays_email_count(service_id: UUID) -> int: def increment_todays_email_count(service_id: UUID, increment_by: int) -> None: if not current_app.config["REDIS_ENABLED"]: return - fetch_todays_email_count(service_id) # to make sure it's set in redis cache_key = email_daily_count_cache_key(service_id) redis_store.incrby(cache_key, increment_by) diff --git a/app/job/rest.py b/app/job/rest.py index 11e009f946..46d7944309 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -22,14 +22,15 @@ from app.dao.templates_dao import dao_get_template_by_id from app.errors import InvalidRequest, register_errors from app.models import ( + EMAIL_TYPE, JOB_STATUS_CANCELLED, JOB_STATUS_PENDING, JOB_STATUS_SCHEDULED, - LETTER_TYPE, SMS_TYPE, ) from app.notifications.process_notifications import simulated_recipient from app.notifications.validators import ( + check_email_limit_increment_redis_send_warnings_if_needed, check_sms_limit_increment_redis_send_warnings_if_needed, ) from app.schemas import ( @@ -144,14 +145,15 @@ def create_job(service_id): data["template"] = data.pop("template_id") template = dao_get_template_by_id(data["template"]) + job = get_job_from_s3(service_id, data["id"]) + recipient_csv = RecipientCSV( + job, + template_type=template.template_type, + placeholders=template._as_utils_template().placeholders, + template=Template(template.__dict__), + ) + if template.template_type == SMS_TYPE: - job = get_job_from_s3(service_id, data["id"]) - recipient_csv = RecipientCSV( - job, - template_type=template.template_type, - placeholders=template._as_utils_template().placeholders, - template=Template(template.__dict__), - ) # calculate the number of simulated recipients numberOfSimulated = sum( simulated_recipient(i["phone_number"].data, template.template_type) for i in list(recipient_csv.get_rows()) @@ -167,8 +169,8 @@ def create_job(service_id): if not is_test_notification: check_sms_limit_increment_redis_send_warnings_if_needed(service, recipient_csv.sms_fragment_count) - if template.template_type == LETTER_TYPE and service.restricted: - raise InvalidRequest("Create letter job is not allowed for service in trial mode ", 403) + if template.template_type == EMAIL_TYPE: + check_email_limit_increment_redis_send_warnings_if_needed(service, len(list(recipient_csv.get_rows()))) if data.get("valid") != "True": raise InvalidRequest("File is not valid, can't create job", 400) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 54b3168921..c065919390 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -7,8 +7,10 @@ from notifications_utils.clients.redis import ( daily_limit_cache_key, near_daily_limit_cache_key, + near_email_daily_limit_cache_key, near_sms_daily_limit_cache_key, over_daily_limit_cache_key, + over_email_daily_limit_cache_key, over_sms_daily_limit_cache_key, rate_limit_cache_key, ) @@ -25,6 +27,7 @@ from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id from app.dao.service_letter_contact_dao import dao_get_letter_contact_by_id from app.dao.service_sms_sender_dao import dao_get_service_sms_senders_by_id +from app.email_limit_utils import fetch_todays_email_count, increment_todays_email_count from app.models import ( EMAIL_TYPE, INTERNATIONAL_SMS_TYPE, @@ -51,9 +54,11 @@ from app.utils import get_document_url, get_public_notify_type_text, is_blank from app.v2.errors import ( BadRequestError, + LiveServiceTooManyEmailRequestsError, LiveServiceTooManyRequestsError, LiveServiceTooManySMSRequestsError, RateLimitError, + TrialServiceTooManyEmailRequestsError, TrialServiceTooManyRequestsError, TrialServiceTooManySMSRequestsError, ) @@ -125,6 +130,58 @@ def check_sms_daily_limit(service: Service, requested_sms=0): raise LiveServiceTooManySMSRequestsError(service.sms_daily_limit) +@statsd_catch( + namespace="validators", + counter_name="rate_limit.trial_service_daily_email", + exception=TrialServiceTooManyEmailRequestsError, +) +@statsd_catch( + namespace="validators", + counter_name="rate_limit.live_service_daily_email", + exception=LiveServiceTooManyEmailRequestsError, +) +def check_email_daily_limit(service: Service, requested_email=0): + emails_sent_today = fetch_todays_email_count(service.id) + bool_over_email_daily_limit = (emails_sent_today + requested_email) > service.message_limit + + # Send a warning when reaching the daily email limit + if not bool_over_email_daily_limit: + return + + current_app.logger.info( + f"service {service.id} is exceeding their daily email limit [total sent today: {int(emails_sent_today)} limit: {service.message_limit}, attempted send: {requested_email}" + ) + if service.restricted: + raise TrialServiceTooManyEmailRequestsError(service.message_limit) + else: + raise LiveServiceTooManyEmailRequestsError(service.message_limit) + + +def send_warning_email_limit_emails_if_needed(service: Service) -> None: + """ + Function that decides if we should send email warnings about nearing or reaching the email daily limit. + """ + todays_current_email_count = fetch_todays_email_count(service.id) + bool_nearing_email_daily_limit = todays_current_email_count >= NEAR_DAILY_LIMIT_PERCENTAGE * service.message_limit + bool_at_or_over_email_daily_limit = todays_current_email_count >= service.message_limit + current_time = datetime.utcnow().isoformat() + cache_expiration = int(time_until_end_of_day().total_seconds()) + + # Send a warning when reaching 80% of the daily limit + if bool_nearing_email_daily_limit: + cache_key = near_email_daily_limit_cache_key(service.id) + if not redis_store.get(cache_key): + send_near_email_limit_email(service) + redis_store.set(cache_key, current_time, ex=cache_expiration) + + # Send a warning when reaching the daily message limit + if bool_at_or_over_email_daily_limit: + cache_key = over_email_daily_limit_cache_key(service.id) + if not redis_store.get(cache_key): + send_email_limit_reached_email(service) + redis_store.set(cache_key, current_time, ex=cache_expiration) + + def send_warning_sms_limit_emails_if_needed(service: Service): todays_requested_sms = fetch_todays_requested_sms_count(service.id) nearing_sms_daily_limit = todays_requested_sms >= NEAR_DAILY_LIMIT_PERCENTAGE * service.sms_daily_limit @@ -167,9 +224,19 @@ def check_sms_limit_increment_redis_send_warnings_if_needed(service: Service, re send_warning_sms_limit_emails_if_needed(service) +def check_email_limit_increment_redis_send_warnings_if_needed(service: Service, requested_email=0) -> None: + if not current_app.config["FF_EMAIL_DAILY_LIMIT"]: + return + + check_email_daily_limit(service, requested_email) + increment_todays_email_count(service.id, requested_email) + send_warning_email_limit_emails_if_needed(service) + + def check_rate_limiting(service: Service, api_key: ApiKey): check_service_over_api_rate_limit_and_update_rate(service, api_key) - check_service_over_daily_message_limit(api_key.key_type, service) + if not current_app.config["FF_EMAIL_DAILY_LIMIT"]: + check_service_over_daily_message_limit(api_key.key_type, service) def warn_about_daily_message_limit(service: Service, messages_sent): @@ -242,6 +309,25 @@ def send_near_sms_limit_email(service: Service): current_app.logger.info(f"service {service.id} is approaching its daily sms limit of {service.sms_daily_limit}") +def send_near_email_limit_email(service: Service) -> None: + """ + Send an email to service users when nearing the daily email limit. + + """ + send_notification_to_service_users( + service_id=service.id, + template_id=current_app.config["NEAR_DAILY_EMAIL_LIMIT_TEMPLATE_ID"], + personalisation={ + "service_name": service.name, + "contact_url": f"{current_app.config['ADMIN_BASE_URL']}/contact", + "message_limit_en": "{:,}".format(service.message_limit), + "message_limit_fr": "{:,}".format(service.message_limit).replace(",", " "), + }, + include_user_fields=["name"], + ) + current_app.logger.info(f"service {service.id} is approaching its daily email limit of {service.message_limit}") + + def send_sms_limit_reached_email(service: Service): send_notification_to_service_users( service_id=service.id, @@ -258,6 +344,20 @@ def send_sms_limit_reached_email(service: Service): ) +def send_email_limit_reached_email(service: Service): + send_notification_to_service_users( + service_id=service.id, + template_id=current_app.config["REACHED_DAILY_EMAIL_LIMIT_TEMPLATE_ID"], + personalisation={ + "service_name": service.name, + "contact_url": f"{current_app.config['ADMIN_BASE_URL']}/contact", + "message_limit_en": "{:,}".format(service.message_limit), + "message_limit_fr": "{:,}".format(service.message_limit).replace(",", " "), + }, + include_user_fields=["name"], + ) + + def check_template_is_for_notification_type(notification_type: NotificationType, template_type: TemplateType): if notification_type != template_type: message = "{0} template is not suitable for {1} notification".format(template_type, notification_type) diff --git a/app/service/rest.py b/app/service/rest.py index f1ecf2fa0c..377ac2adb9 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -329,7 +329,9 @@ def update_service(service_id): def _warn_service_users_about_message_limit_changed(service_id, data): send_notification_to_service_users( service_id=service_id, - template_id=current_app.config["DAILY_LIMIT_UPDATED_TEMPLATE_ID"], + template_id=current_app.config["DAILY_EMAIL_LIMIT_UPDATED_TEMPLATE_ID"] + if current_app.config["FF_EMAIL_DAILY_LIMIT"] + else current_app.config["DAILY_LIMIT_UPDATED_TEMPLATE_ID"], personalisation={ "service_name": data["name"], "message_limit_en": "{:,}".format(data["message_limit"]), diff --git a/app/service/send_notification.py b/app/service/send_notification.py index eddc16b6ad..3e5fc424b8 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -33,6 +33,7 @@ simulated_recipient, ) from app.notifications.validators import ( + check_email_limit_increment_redis_send_warnings_if_needed, check_service_has_permission, check_service_over_daily_message_limit, check_sms_limit_increment_redis_send_warnings_if_needed, @@ -63,11 +64,14 @@ def send_one_off_notification(service_id, post_data): _, template_with_content = validate_template(template.id, personalisation, service, template.template_type) - check_service_over_daily_message_limit(KEY_TYPE_NORMAL, service) + if not current_app.config["FF_EMAIL_DAILY_LIMIT"]: + check_service_over_daily_message_limit(KEY_TYPE_NORMAL, service) if template.template_type == SMS_TYPE: is_test_notification = simulated_recipient(post_data["to"], template.template_type) if not is_test_notification: check_sms_limit_increment_redis_send_warnings_if_needed(service, template_with_content.fragment_count) + elif template.template_type == EMAIL_TYPE and current_app.config["FF_EMAIL_DAILY_LIMIT"]: + check_email_limit_increment_redis_send_warnings_if_needed(service, 1) # 1 email validate_and_format_recipient( send_to=post_data["to"], diff --git a/app/v2/errors.py b/app/v2/errors.py index 39f49d8cda..62187005ea 100644 --- a/app/v2/errors.py +++ b/app/v2/errors.py @@ -38,6 +38,14 @@ def __init__(self, sending_limit): self.message = self.message_template.format(sending_limit) +class TooManyEmailRequestsError(InvalidRequest): + status_code = 429 + messsage_template = "Exceeded email daily sending limit of {} messages" + + def __init__(self, sending_limit): + self.message = self.messsage_template.format(sending_limit) + + class LiveServiceTooManyRequestsError(TooManyRequestsError): pass @@ -46,6 +54,10 @@ class LiveServiceTooManySMSRequestsError(TooManySMSRequestsError): pass +class LiveServiceTooManyEmailRequestsError(TooManyEmailRequestsError): + pass + + class TrialServiceTooManyRequestsError(TooManyRequestsError): pass @@ -54,6 +66,10 @@ class TrialServiceTooManySMSRequestsError(TooManySMSRequestsError): pass +class TrialServiceTooManyEmailRequestsError(TooManyEmailRequestsError): + pass + + class RateLimitError(InvalidRequest): status_code = 429 message_template = "Exceeded rate limit for key type {} of {} requests per {} seconds" diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 167c22543b..ca35ed8689 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -72,6 +72,7 @@ transform_notification, ) from app.notifications.validators import ( + check_email_limit_increment_redis_send_warnings_if_needed, check_rate_limiting, check_service_can_schedule_notification, check_service_email_reply_to_id, @@ -213,6 +214,9 @@ def post_bulk(): ) raise BadRequestError(message=message) + if template.template_type == EMAIL_TYPE and api_user.key_type != KEY_TYPE_TEST: + check_email_limit_increment_redis_send_warnings_if_needed(authenticated_service, len(list(recipient_csv.get_rows()))) + if template.template_type == SMS_TYPE: # calculate the number of simulated recipients numberOfSimulated = sum( @@ -274,6 +278,9 @@ def post_notification(notification_type: NotificationType): notification_type, ) + if template.template_type == EMAIL_TYPE and api_user.key_type != KEY_TYPE_TEST: + check_email_limit_increment_redis_send_warnings_if_needed(authenticated_service, 1) # 1 email + if template.template_type == SMS_TYPE: is_test_notification = api_user.key_type == KEY_TYPE_TEST or simulated_recipient(form["phone_number"], notification_type) if not is_test_notification: diff --git a/migrations/versions/0432_daily_email_limit_templates.py b/migrations/versions/0432_daily_email_limit_templates.py new file mode 100644 index 0000000000..0be21409a1 --- /dev/null +++ b/migrations/versions/0432_daily_email_limit_templates.py @@ -0,0 +1,154 @@ +""" + +Revision ID: 0423_daily_email_limit_updated +Revises: 0422_add_billable_units +Create Date: 2022-09-21 00:00:00 + +""" +from datetime import datetime + +from alembic import op +from flask import current_app + +revision = "0432_daily_email_limit_templates" +down_revision = "0431_add_pt_organisation_type" + +near_email_limit_template_id = current_app.config["NEAR_DAILY_EMAIL_LIMIT_TEMPLATE_ID"] +at_email_limit_template_id = current_app.config["REACHED_DAILY_EMAIL_LIMIT_TEMPLATE_ID"] +daily_email_limit_updated_id = current_app.config["DAILY_EMAIL_LIMIT_UPDATED_TEMPLATE_ID"] + +template_ids = [near_email_limit_template_id, at_email_limit_template_id, daily_email_limit_updated_id] + + +def upgrade(): + template_insert = """ + INSERT INTO templates (id, name, template_type, created_at, content, archived, service_id, subject, + created_by_id, version, process_type, hidden) + VALUES ('{}', '{}', '{}', '{}', '{}', False, '{}', '{}', '{}', 1, '{}', false) + """ + template_history_insert = """ + INSERT INTO templates_history (id, name, template_type, created_at, content, archived, service_id, subject, + created_by_id, version, process_type, hidden) + VALUES ('{}', '{}', '{}', '{}', '{}', False, '{}', '{}', '{}', 1, '{}', false) + """ + + near_email_limit_content = "\n".join( + [ + "Hello ((name)),", + "", + "((service_name)) just reached 80% of its daily email limit of ((message_limit_en)) messages. Your service will be blocked from sending email if you go above the daily limit by the end of the day.", + "", + "You can request a limit increase by [contacting us](((contact_url))).", + "", + "The GC Notify team", + "", + "___", + "", + "Bonjour ((name)),", + "", + "((service_name)) vient d’atteindre 80% de sa limite quotidienne de ((message_limit_fr)) courriels. Votre service ne pourra plus envoyer de courriels si vous allez au-delà de votre limite d’ici la fin de journée.", + "", + "Vous pouvez demander à augmenter cette limite en [nous contactant](((contact_url))).", + "", + "L’équipe GC Notification", + ] + ) + + reached_email_limit_content = "\n".join( + [ + "Hello ((name)),", + "", + "((service_name)) has reached its daily email limit of ((message_limit_en)) messages. Your service has been blocked from sending email messages until tomorrow.", + "", + "You can request a limit increase by [contacting us](((contact_url))).", + "", + "The GC Notify team", + "", + "___", + "", + "Bonjour ((name)),", + "", + "((service_name)) vient d’atteindre sa limite quotidienne de ((message_limit_fr)) courriels. Votre service ne peut plus envoyer de courriels jusqu’à demain.", + "", + "Vous pouvez demander à augmenter cette limite en [nous contactant](((contact_url))).", + "", + "L’équipe GC Notification", + ] + ) + + daily_email_limit_updated_content = "\n".join( + [ + "Hello ((name)),", + "", + "The daily email limit of ((service_name)) has just been updated. You can now send ((message_limit_en)) emails per day. This new limit is effective now.", + "", + "The GC Notify team", + "", + "___", + "", + "Bonjour ((name)),", + "", + "La limite quotidienne de courriels de ((service_name)) a été mise à jour. Vous pouvez désormais envoyer ((message_limit_fr)) courriels par jour. Ce changement est effectif dès maintenant.", + "", + "L’équipe GC Notification", + ] + ) + + templates = [ + { + "id": near_email_limit_template_id, + "name": "Near daily EMAIL limit", + "subject": "Action required: 80% of Daily email sending limit reached for ((service_name)) | Action requise: 80% de la limite d’envoi de courriels quotidienne atteinte pour ((service_name))", + "content": near_email_limit_content, + }, + { + "id": at_email_limit_template_id, + "name": "Daily EMAIL limit reached", + "subject": "Action required: Daily email sending limit reached for ((service_name)) | Action requise: Limite d’envoi de courriels quotidienne atteinte pour ((service_name))", + "content": reached_email_limit_content, + }, + { + "id": daily_email_limit_updated_id, + "name": "Daily EMAIL limit updated", + "subject": "Daily email sending limit updated for ((service_name)) | Limite d’envoi de courriels quotidienne mise à jour pour ((service_name))", + "content": daily_email_limit_updated_content, + }, + ] + + for template in templates: + op.execute( + template_insert.format( + template["id"], + template["name"], + "email", + datetime.utcnow(), + template["content"], + current_app.config["NOTIFY_SERVICE_ID"], + template["subject"], + current_app.config["NOTIFY_USER_ID"], + "normal", + ) + ) + + op.execute( + template_history_insert.format( + template["id"], + template["name"], + "email", + datetime.utcnow(), + template["content"], + current_app.config["NOTIFY_SERVICE_ID"], + template["subject"], + current_app.config["NOTIFY_USER_ID"], + "normal", + ) + ) + + +def downgrade(): + for template_id in template_ids: + op.execute("DELETE FROM notifications WHERE template_id = '{}'".format(template_id)) + op.execute("DELETE FROM notification_history WHERE template_id = '{}'".format(template_id)) + op.execute("DELETE FROM template_redacted WHERE template_id = '{}'".format(template_id)) + op.execute("DELETE FROM templates_history WHERE id = '{}'".format(template_id)) + op.execute("DELETE FROM templates WHERE id = '{}'".format(template_id)) diff --git a/poetry.lock b/poetry.lock index b035be2059..000238ae5f 100644 --- a/poetry.lock +++ b/poetry.lock @@ -2488,7 +2488,7 @@ requests = ">=2.0.0" [[package]] name = "notifications-utils" -version = "52.0.3" +version = "52.0.4" description = "Shared python code for Notification - Provides logging utils etc." category = "main" optional = false @@ -2522,8 +2522,8 @@ werkzeug = "2.3.3" [package.source] type = "git" url = "https://github.com/cds-snc/notifier-utils.git" -reference = "52.0.3" -resolved_reference = "1a1948fd882b90341c6817a87310a7314ed0498a" +reference = "52.0.4" +resolved_reference = "a6ed8d1bc1b70780a204912e4266b1981b330bae" [[package]] name = "ordered-set" @@ -4199,4 +4199,4 @@ testing = ["coverage (>=5.0.3)", "zope.event", "zope.testing"] [metadata] lock-version = "2.0" python-versions = "~3.10.9" -content-hash = "e1080c8f56bd0d5956be98f4a4f2a02b11082a7458f3f440c0e0401cd7761b0f" +content-hash = "b64c41aa4559dd5cd3aa45b775f1511fb4f403f6fa960db5171095af59094371" diff --git a/pyproject.toml b/pyproject.toml index eac631d460..61e154cf46 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -65,7 +65,7 @@ Werkzeug = "2.3.3" MarkupSafe = "2.1.3" # REVIEW: v2 is using sha512 instead of sha1 by default (in v1) itsdangerous = "2.1.2" -notifications-utils = { git = "https://github.com/cds-snc/notifier-utils.git", rev = "52.0.3" } +notifications-utils = { git = "https://github.com/cds-snc/notifier-utils.git", rev = "52.0.4" } # rsa = "4.9 # awscli 1.22.38 depends on rsa<4.8 typing-extensions = "4.5.0" greenlet = "2.0.2" diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 7f2a5538e6..bde5a50db8 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1272,14 +1272,15 @@ def test_save_sms_should_save_default_smm_sender_notification_reply_to_text_on(s persisted_notification = Notification.query.one() assert persisted_notification.reply_to_text == "12345" - def test_should_save_sms_template_to_and_persist_with_job_id(self, sample_job, mocker): + def test_should_save_sms_template_to_and_persist_with_job_id(self, notify_api, sample_job, mocker): notification = _notification_json(sample_job.template, to="+1 650 253 2222", job_id=sample_job.id, row_number=2) mocker.patch("app.celery.provider_tasks.deliver_sms.apply_async") mock_over_daily_limit = mocker.patch("app.celery.tasks.check_service_over_daily_message_limit") notification_id = uuid.uuid4() now = datetime.utcnow() - save_smss(sample_job.template.service_id, [signer_notification.sign(notification)], notification_id) + with set_config_values(notify_api, {"FF_EMAIL_DAILY_LIMIT": False}): + save_smss(sample_job.template.service_id, [signer_notification.sign(notification)], notification_id) persisted_notification = Notification.query.one() assert persisted_notification.to == "+1 650 253 2222" assert persisted_notification.job_id == sample_job.id @@ -1610,7 +1611,9 @@ def test_should_route_save_email_task_to_bulk_on_large_csv_file(self, notify_db_ persisted_notification = Notification.query.one() provider_tasks.deliver_email.apply_async.assert_called_once_with([str(persisted_notification.id)], queue="bulk-tasks") - def test_should_use_email_template_and_persist(self, sample_email_template_with_placeholders, sample_api_key, mocker): + def test_should_use_email_template_and_persist( + self, notify_api, sample_email_template_with_placeholders, sample_api_key, mocker + ): mocker.patch("app.celery.provider_tasks.deliver_email.apply_async") mock_over_daily_limit = mocker.patch("app.celery.tasks.check_service_over_daily_message_limit") @@ -1626,9 +1629,10 @@ def test_should_use_email_template_and_persist(self, sample_email_template_with_ ) with freeze_time("2016-01-01 11:10:00.00000"): - save_emails( - sample_email_template_with_placeholders.service_id, [signer_notification.sign(notification)], notification_id - ) + with set_config_values(notify_api, {"FF_EMAIL_DAILY_LIMIT": False}): + save_emails( + sample_email_template_with_placeholders.service_id, [signer_notification.sign(notification)], notification_id + ) persisted_notification = Notification.query.one() assert persisted_notification.to == "my_email@my_email.com" diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index 8a1532e42e..e184da8199 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -72,6 +72,7 @@ def test_cant_cancel_normal_job(client, sample_job, mocker): assert mock_update.call_count == 0 +@pytest.mark.skip(reason="Letter tests") @freeze_time("2019-06-13 13:00") def test_cancel_letter_job_updates_notifications_and_job_to_cancelled(sample_letter_template, admin_request, mocker): job = create_job(template=sample_letter_template, notification_count=1, job_status="finished") @@ -94,6 +95,7 @@ def test_cancel_letter_job_updates_notifications_and_job_to_cancelled(sample_let assert response == 1 +@pytest.mark.skip(reason="Letter tests") @freeze_time("2019-06-13 13:00") def test_cancel_letter_job_does_not_call_cancel_if_can_letter_job_be_cancelled_returns_False( sample_letter_template, admin_request, mocker @@ -286,6 +288,7 @@ def test_create_job_returns_400_if_file_is_invalid(client, fake_uuid, sample_tem mock_job_dao.assert_not_called() +@pytest.mark.skip(reason="Letter tests") def test_create_job_returns_403_if_letter_template_type_and_service_in_trial( client, fake_uuid, sample_trial_letter_template, mocker ): diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 939703439e..ab14ea786d 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -16,6 +16,8 @@ ApiKeyType, ) from app.notifications.validators import ( + check_email_daily_limit, + check_email_limit_increment_redis_send_warnings_if_needed, check_reply_to, check_service_email_reply_to_id, check_service_letter_contact_id, @@ -34,6 +36,7 @@ from app.v2.errors import ( BadRequestError, RateLimitError, + TooManyEmailRequestsError, TooManyRequestsError, TooManySMSRequestsError, ) @@ -42,6 +45,7 @@ create_sample_notification, create_sample_service, create_sample_service_safelist, + create_sample_template, ) from tests.app.db import ( create_letter_contact, @@ -61,6 +65,8 @@ def enable_redis(notify_api): def count_key(limit_type, service_id): if limit_type == "sms": return f"sms-{service_id}-2016-01-01-count" + elif limit_type == "email": + return f"email-{service_id}-2016-01-01-count" else: return f"{service_id}-2016-01-01-count" @@ -68,6 +74,8 @@ def count_key(limit_type, service_id): def near_key(limit_type, service_id): if limit_type == "sms": return f"nearing-daily-limit-sms-{service_id}-2016-01-01-count" + elif limit_type == "email": + return f"nearing-daily-email-limit-email-{service_id}-2016-01-01-count" else: return f"nearing-{service_id}-2016-01-01-count" @@ -75,36 +83,37 @@ def near_key(limit_type, service_id): def over_key(limit_type, service_id): if limit_type == "sms": return f"over-daily-limit-sms-{service_id}-2016-01-01-count" + elif limit_type == "email": + return f"over-daily-email-limit-email-{service_id}-2016-01-01-count" else: return f"over-{service_id}-2016-01-01-count" -class TestCheckDailyLimits: +class TestCheckDailySMSEmailLimits: @pytest.mark.parametrize( - "limit_type, key_type", - [("all", "test"), ("all", "team"), ("all", "normal"), ("sms", "test"), ("sms", "team"), ("sms", "normal")], + "limit_type", + ["email", "sms"], ) def test_check_service_message_limit_in_cache_with_unrestricted_service_is_allowed( - self, notify_api, limit_type, key_type, sample_service, mocker + self, notify_api, limit_type, sample_service, mocker ): mocker.patch("app.notifications.validators.redis_store.get", return_value=1) mocker.patch("app.notifications.validators.redis_store.set") mocker.patch("app.notifications.validators.services_dao") - if limit_type == "sms": with set_config(notify_api, "FF_SPIKE_SMS_DAILY_LIMIT", True): check_sms_daily_limit(sample_service) else: - check_service_over_daily_message_limit(key_type, sample_service) + check_email_daily_limit(sample_service) app.notifications.validators.redis_store.set.assert_not_called() assert not app.notifications.validators.services_dao.mock_calls @pytest.mark.parametrize( - "limit_type, key_type", - [("all", "test"), ("all", "team"), ("all", "normal"), ("sms", "test"), ("sms", "team"), ("sms", "normal")], + "limit_type", + ["email", "sms"], ) def test_check_service_message_limit_in_cache_under_message_limit_passes( - self, notify_api, limit_type, key_type, sample_service, mocker + self, notify_api, limit_type, sample_service, mocker ): mocker.patch("app.notifications.validators.redis_store.get", return_value=1) mocker.patch("app.notifications.validators.redis_store.set") @@ -113,7 +122,7 @@ def test_check_service_message_limit_in_cache_under_message_limit_passes( with set_config(notify_api, "FF_SPIKE_SMS_DAILY_LIMIT", True): check_sms_daily_limit(sample_service) else: - check_service_over_daily_message_limit(key_type, sample_service) + check_email_daily_limit(sample_service) app.notifications.validators.redis_store.set.assert_not_called() assert not app.notifications.validators.services_dao.mock_calls @@ -192,38 +201,38 @@ def test_check_service_message_limit_over_message_limit_fails( include_user_fields=["name"], ) - def test_check_service_message_limit_records_nearing_daily_limit(self, notify_api, notify_db, notify_db_session, mocker): - limit_type = "all" - email_template = "NEAR_DAILY_LIMIT_TEMPLATE_ID" + @pytest.mark.parametrize( + "limit_type, template_name", + [("email", "NEAR_DAILY_EMAIL_LIMIT_TEMPLATE_ID"), ("sms", "NEAR_DAILY_SMS_LIMIT_TEMPLATE_ID")], + ) + def test_check_service_message_limit_records_nearing_daily_limit( + self, notify_api, limit_type, template_name, notify_db, notify_db_session, mocker + ): with freeze_time("2016-01-01 12:00:00.000000"): - redis_get = mocker.patch("app.redis_store.get", side_effect=[4, None]) - redis_set = mocker.patch("app.redis_store.set") + redis_get = mocker.patch("app.redis_store.get", side_effect=[4, 4, 4, None]) send_notification = mocker.patch("app.notifications.validators.send_notification_to_service_users") service = create_sample_service(notify_db, notify_db_session, restricted=True, limit=5, sms_limit=5) + template = create_sample_template(notify_db, notify_db_session, service=service, template_type=limit_type) for x in range(4): - create_sample_notification(notify_db, notify_db_session, service=service) + create_sample_notification(notify_db, notify_db_session, service=service, template=template) if limit_type == "sms": with set_config(notify_api, "FF_SPIKE_SMS_DAILY_LIMIT", True): check_sms_limit_increment_redis_send_warnings_if_needed(service) else: - check_service_over_daily_message_limit("normal", service) + with set_config(notify_api, "FF_EMAIL_DAILY_LIMIT", True): + check_email_limit_increment_redis_send_warnings_if_needed(service) assert redis_get.call_args_list == [ + call(count_key(limit_type, service.id)), + call(count_key(limit_type, service.id)), call(count_key(limit_type, service.id)), call(near_key(limit_type, service.id)), ] - assert redis_set.call_args_list == [ - call( - near_key(limit_type, service.id), - "2016-01-01T12:00:00", - ex=86400, - ), - ] send_notification.assert_called_once_with( service_id=service.id, - template_id=current_app.config[email_template], + template_id=current_app.config[template_name], personalisation={ "service_name": service.name, "contact_url": f"{current_app.config['ADMIN_BASE_URL']}/contact", @@ -280,6 +289,13 @@ def test_check_service_message_limit_in_cache_over_message_limit_fails( assert e.value.message == "Exceeded SMS daily sending limit of 4 fragments" assert e.value.fields == [] + with pytest.raises(TooManyEmailRequestsError) as e: + with set_config(notify_api, "FF_EMAIL_DAILY_LIMIT", True): + check_email_daily_limit(service) + assert e.value.status_code == 429 + assert e.value.message == "Exceeded email daily sending limit of 4 messages" + assert e.value.fields == [] + app.notifications.validators.redis_store.set.assert_not_called() assert not app.notifications.validators.services_dao.mock_calls @@ -311,7 +327,7 @@ def test_check_service_message_limit_sends_statsd_over_message_limit_fails( app_statsd.statsd_client.incr.assert_called_once_with(expected_counter) - def test_check_service_message_limit_skip_statsd_over_message_no_limit_fails( + def test_check_service_message_limit_skip_statsd_over_message_no_limit_fails_sms( self, notify_api, app_statsd, notify_db, notify_db_session, mocker ): # Given @@ -326,6 +342,20 @@ def test_check_service_message_limit_skip_statsd_over_message_no_limit_fails( # Then app_statsd.statsd_client.incr.assert_not_called() + def test_check_service_message_limit_skip_statsd_over_message_no_limit_fails_emails( + self, notify_api, app_statsd, notify_db, notify_db_session, mocker + ): + # Given + mocker.patch("app.redis_store.get", return_value=0) + mocker.patch("app.notifications.validators.redis_store.set") + + # When + service = create_sample_service(notify_db, notify_db_session, restricted=True, limit=4, sms_limit=4) + with set_config(notify_api, "FF_EMAIL_DAILY_LIMIT", True): + check_email_daily_limit(service) + # Then + app_statsd.statsd_client.incr.assert_not_called() + @pytest.mark.parametrize("template_type, notification_type", [(EMAIL_TYPE, EMAIL_TYPE), (SMS_TYPE, SMS_TYPE)]) def test_check_template_is_for_notification_type_pass(template_type, notification_type): @@ -617,10 +647,12 @@ def test_check_service_sms_sender_id_where_sms_sender_is_not_found(sample_servic assert e.value.message == "sms_sender_id {} does not exist in database for service id {}".format(fake_uuid, sample_service.id) +@pytest.mark.skip(reason="Letter tests") def test_check_service_letter_contact_id_where_letter_contact_id_is_none(): assert check_service_letter_contact_id(None, None, "letter") is None +@pytest.mark.skip(reason="Letter tests") def test_check_service_letter_contact_id_where_letter_contact_id_is_found( sample_service, ): @@ -628,6 +660,7 @@ def test_check_service_letter_contact_id_where_letter_contact_id_is_found( assert check_service_letter_contact_id(sample_service.id, letter_contact.id, LETTER_TYPE) == "123456" +@pytest.mark.skip(reason="Letter tests") def test_check_service_letter_contact_id_where_service_id_is_not_found(sample_service, fake_uuid): letter_contact = create_letter_contact(service=sample_service, contact_block="123456") with pytest.raises(BadRequestError) as e: @@ -638,6 +671,7 @@ def test_check_service_letter_contact_id_where_service_id_is_not_found(sample_se ) +@pytest.mark.skip(reason="Letter tests") def test_check_service_letter_contact_id_where_letter_contact_is_not_found(sample_service, fake_uuid): with pytest.raises(BadRequestError) as e: check_service_letter_contact_id(sample_service.id, fake_uuid, LETTER_TYPE) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index cab6bb41c1..a4727ce674 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2225,7 +2225,9 @@ def test_update_service_updating_daily_limit_sends_notification_to_users( if expected_call: send_notification_mock.assert_called_once_with( service_id=service.id, - template_id=current_app.config["DAILY_LIMIT_UPDATED_TEMPLATE_ID"], + template_id=current_app.config["DAILY_EMAIL_LIMIT_UPDATED_TEMPLATE_ID"] + if current_app.config["FF_EMAIL_DAILY_LIMIT"] + else current_app.config["DAILY_LIMIT_UPDATED_TEMPLATE_ID"], personalisation={ "service_name": service.name, "message_limit_en": "{:,}".format(new_limit), diff --git a/tests/app/service/test_send_one_off_notification.py b/tests/app/service/test_send_one_off_notification.py index 73704366fd..38f129b578 100644 --- a/tests/app/service/test_send_one_off_notification.py +++ b/tests/app/service/test_send_one_off_notification.py @@ -18,6 +18,7 @@ from app.service.send_notification import send_one_off_notification from app.v2.errors import ( BadRequestError, + LiveServiceTooManyEmailRequestsError, LiveServiceTooManySMSRequestsError, TooManyRequestsError, ) @@ -29,6 +30,7 @@ create_template, create_user, ) +from tests.conftest import set_config_values @pytest.fixture @@ -270,6 +272,25 @@ def test_send_one_off_notification_raises_if_over_combined_limit(notify_db_sessi send_one_off_notification(service.id, post_data) +def test_send_one_off_notification_raises_if_over_email_limit(notify_db_session, notify_api, mocker): + service = create_service(message_limit=0) + template = create_template(service=service, template_type=EMAIL_TYPE) + mocker.patch( + "app.service.send_notification.check_email_limit_increment_redis_send_warnings_if_needed", + side_effect=LiveServiceTooManyEmailRequestsError(1), + ) + + post_data = { + "template_id": str(template.id), + "to": "6502532222", + "created_by": str(service.created_by_id), + } + + with set_config_values(notify_api, {"FF_EMAIL_DAILY_LIMIT": True}): + with pytest.raises(LiveServiceTooManyEmailRequestsError): + send_one_off_notification(service.id, post_data) + + def test_send_one_off_notification_raises_if_over_sms_daily_limit(notify_db_session, mocker): service = create_service(sms_daily_limit=0) template = create_template(service=service)