Reinstate bulk sms limit (#294)

* Reinstate PR #278 which added additional length checks to bulk SMS sends (#293)"

This reverts commit 0146718.

* Reinstate "Exclude content length errors from recipient_error prop (#290)" (#292)"

This reverts commit cd287cf.

* Remove unused parameter

* Fix params

* Bump utils version

.github/actions/waffles/requirements.txt

@@ -1,4 +1,4 @@
 docopt==0.6.2
 Flask==2.3.3
 markupsafe==2.1.5
-git+https://github.com/cds-snc/[email protected].3#egg=notifications-utils
+git+https://github.com/cds-snc/[email protected].4#egg=notifications-utils

notifications_utils/columns.py

@@ -59,6 +59,9 @@ def __init__(
             else ["phone number", "numéro de téléphone", "to"]
         )
 
+        # This won't mark a row as too long in all cases. A message can be too long if
+        # placeholder content is added by a user that exceeds the limit when added to
+        # the template's content.
         if template:
             template.values = row_dict
             self.message_too_long = template.is_message_too_long()
@@ -139,4 +142,9 @@ def __eq__(self, other):
 
     @property
     def recipient_error(self):
+        # TODO: This is a bandaid solution. We need to establish why we are calling this Cell property on
+        #       Cells that do not represent a recipient value.
+        if self.error is not None and "Some messages may be too long due to custom content." in self.error:
+            return False
+
         return self.error not in {None, self.missing_field_error}

notifications_utils/recipients.py

@@ -10,7 +10,9 @@
 from collections import OrderedDict, namedtuple
 from ordered_set import OrderedSet
 from typing import Callable, Dict, List
+from notifications_utils import SMS_CHAR_COUNT_LIMIT
 from flask import current_app
+from notifications_utils.sanitise_text import SanitiseSMS
 from . import EMAIL_REGEX_PATTERN, hostname_part, tld_part
 from notifications_utils.formatters import strip_and_remove_obscure_whitespace, strip_whitespace
 from notifications_utils.template import SMSMessageTemplate, Template
@@ -274,6 +276,23 @@ def rows_with_missing_data(self):
     def rows_with_message_too_long(self):
         return self._filter_rows("message_too_long")
 
+    @property
+    def rows_with_combined_variable_content_too_long(self):
+        """
+        Checks if the length of all variable, plus the length of the template content,
+        exceeds the SMS limit of 612 characters. Counts non-GSM characters as 2.
+        """
+        if self.rows_as_list:
+            for row in self.rows_as_list:
+                if row.personalisation and self.template:
+                    variable_length = 0
+                    for variable in row.personalisation.as_dict_with_keys(self.template.placeholders).values():
+                        if variable:
+                            variable_length += sum(1 if char in SanitiseSMS.ALLOWED_CHARACTERS else 2 for char in str(variable))
+                    total_length = variable_length + (len(self.template.content) if self.template else 0)
+                    if total_length > SMS_CHAR_COUNT_LIMIT:
+                        yield row
+
     @property
     def initial_rows_with_errors(self):
         return islice(self.rows_with_errors, self.max_errors_shown)
@@ -359,7 +378,9 @@ def _get_error_for_field(self, key, value):  # noqa: C901
         if self.is_optional_address_column(key):
             return
 
-        if Columns.make_key(key) in self.recipient_column_headers_as_column_keys:
+        formatted_key = Columns.make_key(key)
+
+        if formatted_key in self.recipient_column_headers_as_column_keys:
             if value in [None, ""] or isinstance(value, list):
                 if self.duplicate_recipient_column_headers:
                     return None
@@ -370,12 +391,19 @@ def _get_error_for_field(self, key, value):  # noqa: C901
             except (InvalidEmailError, InvalidPhoneError, InvalidAddressError) as error:
                 return str(error)
 
-        if Columns.make_key(key) not in self.placeholders_as_column_keys:
+        if formatted_key not in self.placeholders_as_column_keys:
             return
 
         if value in [None, ""]:
             return Cell.missing_field_error
 
+        if self.template:
+            if formatted_key in self.placeholders_as_column_keys and self.template.template_type == "sms":
+                try:
+                    validate_sms_message_length(value, self.template.content)
+                except ValueError as error:
+                    return str(error)
+
 
 class InvalidEmailError(Exception):
     def __init__(self, message=None):
@@ -551,6 +579,14 @@ def validate_recipient(recipient, template_type: str, column=None, international
     return validators[template_type](recipient, column)
 
 
+def validate_sms_message_length(variable_content, template_content_length):
+    variable_length = sum(1 if char in SanitiseSMS.ALLOWED_CHARACTERS else 2 for char in variable_content)
+
+    if variable_length + len(template_content_length) > SMS_CHAR_COUNT_LIMIT:
+        raise ValueError(f"Maximum {SMS_CHAR_COUNT_LIMIT} characters. Some messages may be too long due to custom content.")
+    return
+
+
 @lru_cache(maxsize=32, typed=False)
 def format_recipient(recipient):
     if not isinstance(recipient, str):

pyproject.toml

@@ -5,7 +5,7 @@ include = '(notifications_utils|tests)/.*\.pyi?$'
 
 [tool.poetry]
 name = "notifications-utils"
-version = "52.2.3"
+version = "52.2.4"
 description = "Shared python code for Notification - Provides logging utils etc."
 authors = ["Canadian Digital Service"]
 license = "MIT license"

tests/test_recipient_csv.py

@@ -1,3 +1,4 @@
+from math import floor
 import pytest
 import itertools
 import unicodedata
@@ -745,29 +746,57 @@ def test_recipient_safelist(file_contents, template_type, safelist, count_of_row
     assert recipients.allowed_to_send_to
 
 
-def test_detects_rows_which_result_in_overly_long_messages():
+@pytest.mark.parametrize(
+    "template_content, csv, error_rows",
+    [
+        (
+            "((placeholder))",
+            f"""
+                phone number,placeholder
+                6502532222,1
+                6502532222,{"a" * (SMS_CHAR_COUNT_LIMIT - 1)}
+                6502532223,{"a" * SMS_CHAR_COUNT_LIMIT}
+                6502532224,{"a" * (SMS_CHAR_COUNT_LIMIT + 1)}
+            """,
+            {3},
+        ),
+        (  # Placeholder + content length should not exceed SMS_CHAR_COUNT_LIMIT. 72 = content length - placeholder text
+            "((placeholder)) This is template content. Believe it or not there are character limits.",
+            f"""
+            phone number,placeholder
+            6502532222,1
+            6502532222,{'a' * (SMS_CHAR_COUNT_LIMIT + 1)}
+            6502532222,{'a'* (SMS_CHAR_COUNT_LIMIT - 72)}
+            6502532222,{'a' * (SMS_CHAR_COUNT_LIMIT - 73) }
+            """,
+            {1},
+        ),
+        (
+            "((placeholder1)) This is template content.((placeholder2)) Believe it or not there are character limits.",
+            f"""
+            phone number,placeholder1,placeholder2
+            6502532222,1
+            6502532222,{'a' * (floor(SMS_CHAR_COUNT_LIMIT / 2))},{'a' * (floor(SMS_CHAR_COUNT_LIMIT / 2) + 1)}
+            6502532222,{'a'* (floor(SMS_CHAR_COUNT_LIMIT / 2))},{'a' * (floor(SMS_CHAR_COUNT_LIMIT / 2) - 72)}
+            6502532222,{'a' * (floor(SMS_CHAR_COUNT_LIMIT / 2)) },{'a' * (floor(SMS_CHAR_COUNT_LIMIT / 2) - 73)}
+            """,
+            {1},
+        ),
+    ],
+)
+def test_detects_rows_which_result_in_overly_long_messages(template_content, csv, error_rows):
     template = SMSMessageTemplate(
-        {"content": "((placeholder))", "template_type": "sms"},
+        {"content": template_content, "template_type": "sms"},
         sender=None,
         prefix=None,
     )
     recipients = RecipientCSV(
-        """
-            phone number,placeholder
-            6502532222,1
-            6502532222,{one_under}
-            6502532223,{exactly}
-            6502532224,{one_over}
-        """.format(
-            one_under="a" * (SMS_CHAR_COUNT_LIMIT - 1),
-            exactly="a" * SMS_CHAR_COUNT_LIMIT,
-            one_over="a" * (SMS_CHAR_COUNT_LIMIT + 1),
-        ),
+        csv,
         template_type=template.template_type,
         template=template,
     )
-    assert _index_rows(recipients.rows_with_errors) == {3}
-    assert _index_rows(recipients.rows_with_message_too_long) == {3}
+    assert _index_rows(recipients.rows_with_errors) == error_rows
+    assert _index_rows(recipients.rows_with_message_too_long) == error_rows
     assert recipients.has_errors