Reinstate bulk sms limit (#1834)

* Reinstate "Add validation for SMS sends via CSV that exceed 612" (#1822)" This reverts commit 1eec27f. * Reinstate bulk sms limit validation - Bumped boto3 so we have compatibility with latest utils - Added a check to utils.py to ensure we only return validation errors for SMS bulk sends, not email * Point utils to the corresponding branch for now * (Chore) Update lock file * Fix SMS template type check * Fix tests * formatting... * Bump utils version --------- Co-authored-by: Jumana B <[email protected]>
cds-snc · May 22, 2024 · 23196f1 · 23196f1
1 parent be7bc71
commit 23196f1
Show file tree

Hide file tree

Showing 7 changed files with 102 additions and 14 deletions.
diff --git a/app/main/views/send.py b/app/main/views/send.py
@@ -688,6 +688,7 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_
     )
     recipients = RecipientCSV(
         contents,
+        template=template,
         template_type=template.template_type,
         placeholders=template.placeholders,
         max_initial_rows_shown=int(request.args.get("show") or 10),

diff --git a/app/translations/csv/fr.csv b/app/translations/csv/fr.csv
@@ -1815,6 +1815,8 @@
 "For other questions about GC Notify, use this form.","Pour toute autre question, utilisez le formulaire ci-dessous."
 "We answer many questions on other GC Notify pages:","Nous répondons à de nombreuses questions sur les autres pages de Notification GC :"
 "Register for a demo","S’inscrire à une démo"
+"added custom content exceeds the {} character limit in 1 row","Le contenu personnalisé ajouté dépasse la limite de {} caractères pour 1 ligne"
+"added custom content exceeds the {} character limit in {} rows","Le contenu personnalisé ajouté dépasse la limite de {} caractères pour {} lignes"
 "Select another logo","Sélectionner un autre logo"
 "Select alternate logo","Sélectionner un autre logo"
 "Name of logo","Nom du logo"

diff --git a/app/utils.py b/app/utils.py
@@ -24,6 +24,7 @@
 from flask_babel import _
 from flask_babel import lazy_gettext as _l
 from flask_login import current_user, login_required
+from notifications_utils import SMS_CHAR_COUNT_LIMIT
 from notifications_utils.field import Field
 from notifications_utils.formatters import make_quotes_smart
 from notifications_utils.letter_timings import letter_can_be_cancelled
@@ -45,6 +46,7 @@
 from werkzeug.routing import RequestRedirect
 
 from app import cache
+from app.models.enum.template_types import TemplateType
 from app.notify_client.organisations_api_client import organisations_client
 from app.notify_client.service_api_client import service_api_client
 from app.types import EmailReplyTo
@@ -208,6 +210,18 @@ def get_errors_for_csv(recipients, template_type):
         else:
             errors.append(_("enter missing data in {} rows").format(number_of_rows_with_missing_data))
 
+    if recipients.template_type == TemplateType.SMS.value and any(recipients.rows_with_combined_variable_content_too_long):
+        num_rows_with_combined_content_too_long = len(list(recipients.rows_with_combined_variable_content_too_long))
+        if num_rows_with_combined_content_too_long == 1:
+            errors.append(_("added custom content exceeds the {} character limit in 1 row").format(SMS_CHAR_COUNT_LIMIT))
+        else:
+            errors.append(
+                _("added custom content exceeds the {} character limit in {} rows").format(
+                    SMS_CHAR_COUNT_LIMIT, num_rows_with_combined_content_too_long
+                )
+            )
+        # TODO Update the inline cell error messages
+
     return errors
 
 

diff --git a/poetry.lock b/poetry.lock
diff --git a/pyproject.toml b/pyproject.toml
@@ -53,7 +53,7 @@ unidecode = "^1.3.6"
 
 # PaaS
 awscli-cwlogs = "^1.4.6"
-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" }
 
 # Pinned dependencies
 rsa = "^4.1" # not directly required, pinned by Snyk to avoid a vulnerability

diff --git a/tests/app/main/test_errors_for_csv.py b/tests/app/main/test_errors_for_csv.py
@@ -4,37 +4,45 @@
 
 from app.utils import get_errors_for_csv
 
-MockRecipients = namedtuple("MockRecipients", ["rows_with_bad_recipients", "rows_with_missing_data"])
+MockRecipients = namedtuple(
+    "MockRecipients",
+    ["rows_with_bad_recipients", "rows_with_missing_data", "rows_with_combined_variable_content_too_long", "template_type"],
+)
 
 
 @pytest.mark.parametrize(
-    "rows_with_bad_recipients,rows_with_missing_data,template_type,expected_errors",
+    "rows_with_bad_recipients, rows_with_missing_data, rows_with_combined_variable_content_too_long, template_type, expected_errors",
     [
-        ([], [], "sms", []),
-        ({2}, [], "sms", ["fix 1 phone number"]),
-        ({2, 4, 6}, [], "sms", ["fix 3 phone numbers"]),
-        ({1}, [], "email", ["fix 1 email address"]),
-        ({2, 4, 6}, [], "email", ["fix 3 email addresses"]),
-        ({2}, {3}, "sms", ["fix 1 phone number", "enter missing data in 1 row"]),
+        ([], [], [], "sms", []),
+        ({2}, [], [], "sms", ["fix 1 phone number"]),
+        ({2, 4, 6}, [], [], "sms", ["fix 3 phone numbers"]),
+        ({1}, [], [], "email", ["fix 1 email address"]),
+        ({2, 4, 6}, [], [], "email", ["fix 3 email addresses"]),
+        ({2}, {3}, [], "sms", ["fix 1 phone number", "enter missing data in 1 row"]),
         (
             {2, 4, 6, 8},
             {3, 6, 9, 12},
+            [],
             "sms",
             ["fix 4 phone numbers", "enter missing data in 4 rows"],
         ),
+        ([], [], {2, 5, 8}, "sms", ["added custom content exceeds the 612 character limit in 3 rows"]),
     ],
 )
 def test_get_errors_for_csv(
     app_,
     rows_with_bad_recipients,
     rows_with_missing_data,
+    rows_with_combined_variable_content_too_long,
     template_type,
     expected_errors,
 ):
     with app_.test_request_context():
         assert (
             get_errors_for_csv(
-                MockRecipients(rows_with_bad_recipients, rows_with_missing_data),
+                MockRecipients(
+                    rows_with_bad_recipients, rows_with_missing_data, rows_with_combined_variable_content_too_long, template_type
+                ),
                 template_type,
             )
             == expected_errors

diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
@@ -522,6 +522,69 @@ def test_upload_csv_file_with_missing_columns_shows_error(
     assert normalize_spaces(page.select_one("h1").text) == expected_heading
 
 
+@pytest.mark.parametrize(
+    "file_contents, expected_error, expected_heading, num_cells_errors",
+    [
+        (
+            f"""
+                phone number, one, two, three
+                +16502532222, {'a' * 155}, {'a' * 155}, {'a' * 155}
+                +16502532222, {'a' * 613}, {'a' * 613}, {'a' * 613}
+                +16502532222, {'a' * 50}, {'a' * 50}, {'a' * 50}
+            """,
+            "Maximum 612 characters. Some messages may be too long due to custom content.",
+            "Added custom content exceeds the 612 character limit in 1 row",
+            3,
+        ),
+        (
+            f"""
+                phone number, one, two, three
+                +16502532222, {'a' * 613}, {'a' * 613}, {'a' * 613}
+                +16502532222, {'a' * 619}, {'a' * 619}, {'a' * 619}
+                +16502532222, {'a' * 700}, {'a' * 700}, {'a' * 700}
+            """,
+            "Maximum 612 characters. Some messages may be too long due to custom content.",
+            "Added custom content exceeds the 612 character limit in 3 rows",
+            9,
+        ),
+    ],
+)
+def test_upload_csv_variables_too_long_shows_banner_and_inline_cell_errors(
+    client_request,
+    mocker,
+    mock_get_service_template_with_multiple_placeholders,
+    mock_s3_upload,
+    mock_get_users_by_service,
+    mock_get_service_statistics,
+    mock_get_template_statistics,
+    mock_get_job_doesnt_exist,
+    mock_get_jobs,
+    service_one,
+    fake_uuid,
+    file_contents,
+    expected_error,
+    expected_heading,
+    num_cells_errors,
+):
+    mocker.patch("app.main.views.send.s3download", return_value=file_contents)
+    page = client_request.post(
+        "main.send_messages",
+        service_id=service_one["id"],
+        template_id=fake_uuid,
+        _data={"file": (BytesIO("".encode("utf-8")), "invalid.csv")},
+        _follow_redirects=True,
+    )
+    cell_errors = page.find_all("span", class_="table-field-error-label")
+
+    with client_request.session_transaction() as session:
+        assert "file_uploads" not in session
+
+    assert normalize_spaces(page.find(role="alert").text) == expected_heading
+    assert normalize_spaces(page.select_one("h1").text) == "Edit your spreadsheet"
+    assert all(cell.text == expected_error for cell in cell_errors)
+    assert len(cell_errors) == num_cells_errors
+
+
 def test_upload_csv_invalid_extension(
     logged_in_client,
     mock_login,