From 6919e3c739d243f7b89ad9e8f86999486b5504eb Mon Sep 17 00:00:00 2001 From: William B <7444334+whabanks@users.noreply.github.com> Date: Wed, 22 May 2024 16:10:17 -0400 Subject: [PATCH] Reinstate bulk sms limit (#2169) * Reinstate "Add error message for rows_with_combined_variable_content_too_long" (#2164)" This reverts commit 5b29366eeb563c81f7306c68ad41b9a0e7802bf7. * Reinstate bulk sms limit validation - Added an additional check to ensure that we only return an 4xx response if the template type is SMS * Bump utils version * Refresh lock file --- app/v2/notifications/post_notifications.py | 7 +++++++ poetry.lock | 8 ++++---- pyproject.toml | 2 +- tests/app/celery/test_tasks.py | 14 +++++++++----- .../v2/notifications/test_post_notifications.py | 4 ++-- 5 files changed, 23 insertions(+), 12 deletions(-) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index b1fc62e447..d8edf8627b 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -7,6 +7,7 @@ import werkzeug from flask import abort, current_app, jsonify, request +from notifications_utils import SMS_CHAR_COUNT_LIMIT from notifications_utils.recipients import ( RecipientCSV, try_validate_and_format_phone_number, @@ -705,6 +706,12 @@ def check_for_csv_errors(recipient_csv, max_rows, remaining_messages): message=f"You cannot send to these recipients {explanation}", status_code=400, ) + if recipient_csv.template_type == SMS_TYPE and any(recipient_csv.rows_with_combined_variable_content_too_long): + raise BadRequestError( + message=f"Row {next(recipient_csv.rows_with_combined_variable_content_too_long).index + 1} - has a character count greater than {SMS_CHAR_COUNT_LIMIT} characters. Some messages may be too long due to custom content.", + status_code=400, + ) + if recipient_csv.rows_with_errors: def row_error(row): diff --git a/poetry.lock b/poetry.lock index 67aa854ed1..8209d2ed6f 100644 --- a/poetry.lock +++ b/poetry.lock @@ -2444,7 +2444,7 @@ requests = ">=2.0.0" [[package]] name = "notifications-utils" -version = "52.2.3" +version = "52.2.4" description = "Shared python code for Notification - Provides logging utils etc." optional = false python-versions = "~3.10.9" @@ -2479,8 +2479,8 @@ werkzeug = "2.3.7" [package.source] type = "git" url = "https://github.com/cds-snc/notifier-utils.git" -reference = "52.2.3" -resolved_reference = "fc02dace174f93072345160787b2e603b74a984b" +reference = "52.2.4" +resolved_reference = "1e2c279333ee1b86671b82d8f562bb3e98446500" [[package]] name = "ordered-set" @@ -4212,4 +4212,4 @@ testing = ["coverage (>=5.0.3)", "zope.event", "zope.testing"] [metadata] lock-version = "2.0" python-versions = "~3.10.9" -content-hash = "ca9fbdd9131c2decb054fb5d50ffb7d5fe4aa88cf0f4264e17b2ef1a1486f5b8" +content-hash = "62653f4a581d32ac1678c8454f1100320096e8166aca1599cffd6fd3f72cfb4b" diff --git a/pyproject.toml b/pyproject.toml index 4107d6a732..9844f1c756 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -64,7 +64,7 @@ Werkzeug = "2.3.7" MarkupSafe = "2.1.5" # 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", tag = "52.2.3" } +notifications-utils = { git = "https://github.com/cds-snc/notifier-utils.git", tag = "52.2.4" } # rsa = "4.9 # awscli 1.22.38 depends on rsa<4.8 typing-extensions = "4.7.1" greenlet = "2.0.2" diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index a2f0c367a7..aeeb6c8c76 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -2,7 +2,7 @@ import uuid from datetime import datetime, timedelta from unittest import mock -from unittest.mock import Mock, call +from unittest.mock import MagicMock, Mock, call import pytest import requests_mock @@ -891,9 +891,10 @@ def test_process_rows_sends_save_task( mocker.patch("app.celery.tasks.create_uuid", return_value="noti_uuid") task_mock = mocker.patch("app.celery.tasks.{}".format(expected_function)) signer_mock = mocker.patch("app.celery.tasks.signer_notification.sign") - template = Mock(id="template_id", template_type=template_type, process_type=NORMAL) + template = MagicMock(id="template_id", template_type=template_type, process_type=NORMAL) job = Mock(id="job_id", template_version="temp_vers", notification_count=1, api_key_id=api_key_id, sender_id=sender_id) service = Mock(id="service_id", research_mode=research_mode) + template.__len__.return_value = 1 process_rows( [ @@ -950,10 +951,11 @@ def test_should_redirect_email_job_to_queue_depending_on_csv_threshold( ): mock_save_email = mocker.patch("app.celery.tasks.save_emails") - template = Mock(id=1, template_type=EMAIL_TYPE, process_type=template_process_type) + template = MagicMock(id=1, template_type=EMAIL_TYPE, process_type=template_process_type) api_key = Mock(id=1, key_type=KEY_TYPE_NORMAL) job = Mock(id=1, template_version="temp_vers", notification_count=1, api_key=api_key) service = Mock(id=1, research_mode=False) + template.__len__.return_value = 1 row = next( RecipientCSV( @@ -994,10 +996,11 @@ def test_should_redirect_sms_job_to_queue_depending_on_csv_threshold( ): mock_save_sms = mocker.patch("app.celery.tasks.save_smss") - template = Mock(id=1, template_type=SMS_TYPE, process_type=template_process_type) + template = MagicMock(id=1, template_type=SMS_TYPE, process_type=template_process_type) api_key = Mock(id=1, key_type=KEY_TYPE_NORMAL) job = Mock(id=1, template_version="temp_vers", notification_count=1, api_key=api_key) service = Mock(id=1, research_mode=False) + template.__len__.return_value = 1 row = next( RecipientCSV( @@ -1066,7 +1069,8 @@ def test_process_rows_works_without_key_type( mocker.patch("app.celery.tasks.create_uuid", return_value="noti_uuid") task_mock = mocker.patch("app.celery.tasks.{}".format(expected_function)) signer_mock = mocker.patch("app.celery.tasks.signer_notification.sign") - template = Mock(id="template_id", template_type=template_type, process_type=NORMAL) + template = MagicMock(id="template_id", template_type=template_type, process_type=NORMAL) + template.__len__.return_value = 1 api_key = {} job = Mock( id="job_id", diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 93b81c47cd..cc33c4d527 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -2523,7 +2523,7 @@ def test_post_bulk_with_too_large_sms_fails(self, client, notify_db, notify_db_s mocker.patch("app.v2.notifications.post_notifications.create_bulk_job", return_value=str(uuid.uuid4())) service = create_service(sms_daily_limit=10, message_limit=100) - template = create_sample_template(notify_db, notify_db_session, service=service, template_type="sms", content="a" * 612) + template = create_sample_template(notify_db, notify_db_session, service=service, template_type="sms", content="a" * 613) data = { "name": "job_name", "template_id": template.id, @@ -2574,7 +2574,7 @@ def test_post_bulk_with_too_large_sms_fail_and_shows_correct_row( ) assert response.status_code == 400 assert "has a character count greater than" in str(response.data) - assert "row #{}".format(failure_row) in str(response.data) + assert "Row {}".format(failure_row) in str(response.data) class TestBatchPriorityLanes: