Validate variables in CSV do not exceed daily limit (#278)

* Validate variables in CSV do not exceed daily limit - Added has_message_too_long property to the Row class - Added message_to_long variable to the Cell class with accompanying property content_length_error to identify a cell with the aformentioned error - Added a check to the RecipientCSV class that checks if a variable in a CSV exceeds the message limit of 612 when added to the template content * Add validate_sms_message_length - Raise an execption when length is exceeded - Bubble exception message up to the UI for rendering * Check if combined variable content exceeds sms limit * Add tests - Removed message_too_long_error from the Cell class * Remove unneeded property * Use SMS_CHAR_COUNT_LIMIT instead of hard coded value Co-authored-by: Jimmy Royer <[email protected]> * Use SMS_CHAR_COUNT_LIMIT instead of hard coded value * Use generators properly - Updated rows_with_combined_variable_content_too_long make use of generators properly - Removed None check * Bump utils + waffles version --------- Co-authored-by: Jimmy Royer <[email protected]>
cds-snc · Mar 11, 2024 · 7385bbc · 7385bbc
1 parent c0549f0
commit 7385bbc
Show file tree

Hide file tree

Showing 5 changed files with 91 additions and 23 deletions.
diff --git a/.github/actions/waffles/requirements.txt b/.github/actions/waffles/requirements.txt
@@ -1,4 +1,4 @@
 docopt==0.6.2
 Flask==2.3.3
 markupsafe==2.1.4
-git+https://github.com/cds-snc/[email protected].5#egg=notifications-utils
+git+https://github.com/cds-snc/[email protected].6#egg=notifications-utils
diff --git a/notifications_utils/columns.py b/notifications_utils/columns.py
@@ -63,7 +63,12 @@ def __init__(
             template.values = row_dict
             self.message_too_long = template.is_message_too_long()
 
-        super().__init__(OrderedDict((key, Cell(key, value, error_fn, self.placeholders)) for key, value in row_dict.items()))
+        super().__init__(
+            OrderedDict(
+                (key, Cell(key, value, error_fn, self.placeholders, len(template.content) if template else None))
+                for key, value in row_dict.items()
+            )
+        )
 
     def __getitem__(self, key):
         return super().__getitem__(key) or Cell()
@@ -121,7 +126,7 @@ def recipient_and_personalisation(self):
 class Cell:
     missing_field_error = "Missing"
 
-    def __init__(self, key=None, value=None, error_fn=None, placeholders=None):
+    def __init__(self, key=None, value=None, error_fn=None, placeholders=None, template_content_length=None):
         self.data = value
         self.error = error_fn(key, value) if error_fn else None
         self.ignore = Columns.make_key(key) not in (placeholders or [])

diff --git a/notifications_utils/recipients.py b/notifications_utils/recipients.py
@@ -10,9 +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
@@ -276,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)
@@ -361,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
@@ -372,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):
@@ -553,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):

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

diff --git a/tests/test_recipient_csv.py b/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