#127 Bugfix: Email Preview Exposes HTML Tags

README.md

@@ -18,9 +18,11 @@ The `./scripts/run_tests.sh` script runs all unit tests using [py.test](http://p
 
 ## Versioning
 
-With the virtual environment active, run `python setup.py --version` to see the current version **on the current branch**.  Use `git tag` to add release tags to commits, if desired, and push the tags.
+With the virtual environment active, run `python setup.py --version` to see the current version **on the current branch**.  
 
-After merging changes in this repository, you must update notification-api to use the changes.  Run `poetry update notification-utils` in an api branch, and then push the PR for approval/merge. The PR only should contain changes made to the lock file.
+Before merging, update the version in the `notifications_utils/version.py` file. Once merged, use `git tag` to add release tags to commits, and push the tags.
+
+After merging changes in this repository, you must update notification-api to use the changes. Run `poetry update notification-utils` in an api branch, and then push the PR for approval/merge. The PR only should contain changes made to the lock file.
 
 ## E-mail Template Documentation
 

notifications_utils/formatters.py

@@ -299,7 +299,7 @@ def get_action_links(html: str) -> list[str]:
     return re.findall(action_link_regex, html)
 
 
-def get_img_link() -> str:
+def get_action_link_image_url() -> str:
     """Get action link image url for the current environment. (insert_action_link helper)"""
     env_map = {
         'production': 'prod',
@@ -322,7 +322,7 @@ def insert_action_link(html: str) -> str:
 
     action_link_list = get_action_links(html)
 
-    img_link = get_img_link()
+    img_link = get_action_link_image_url()
 
     for item in action_link_list:
         # Puts the action link in a new <p> tag with appropriate styling.
@@ -365,6 +365,61 @@ def insert_action_link(html: str) -> str:
     return html
 
 
+def strip_parentheses_in_link_placeholders(value: str) -> str:
+    """
+    Captures markdown links with placeholders in them and replaces the parentheses around the placeholders with
+    !! at the start and ## at the end. This makes them easy to put back after the convertion to html.
+
+    Example Conversion:
+    `[link text](http://example.com/((placeholder))) -> [link text](http://example.com/!!placeholder##)`
+
+    Args:
+        value (str): The email body to be processed
+
+    Returns:
+        str: The email body with the placeholders in markdown links with parentheses replaced with !! and ##
+    """
+    markdown_link_pattern = re.compile(r'\]\((.*?\({2}.*?\){2}.*?)+?\)')
+
+    # find all markdown links with placeholders in them and replace the parentheses and html tags with !! and ##
+    for item in re.finditer(markdown_link_pattern, value):
+        link = item.group(0)
+        # replace the opening parentheses with !!, include the opening span and mark tags if they exist
+        modified_link = re.sub(r'((<span class=[\'\"]placeholder[\'\"]><mark>)?\(\((?![\(]))', '!!', link)
+        # replace the closing parentheses with ##, include the closing span and mark tags if they exist
+        modified_link = re.sub(r'(\)\)(<\/mark><\/span>)?)', '##', modified_link)
+
+        value = value.replace(link, modified_link)
+
+    return value
+
+
+def replace_symbols_with_placeholder_parens(value: str) -> str:
+    """
+    Replaces the `!!` and `##` symbols with placeholder parentheses in the given string.
+
+    Example Output: `!!placeholder## -> ((placeholder))`
+
+    Args:
+        value (str): The email body that has been converted from markdown to html
+
+    Returns:
+        str: The processed string with tags replaced by placeholder parentheses.
+    """
+    # pattern to find the placeholders surrounded by !! and ##
+    placeholder_in_link_pattern = re.compile(r'(!![^()]+?##)')
+
+    # find all instances of !! and ## and replace them with (( and ))
+    for item in re.finditer(placeholder_in_link_pattern, value):
+        placeholder = item.group(0)
+        mod_placeholder = placeholder.replace('!!', '((')
+        mod_placeholder = mod_placeholder.replace('##', '))')
+
+        value = value.replace(placeholder, mod_placeholder)
+
+    return value
+
+
 class NotifyLetterMarkdownPreviewRenderer(mistune.Renderer):
 
     def block_code(self, code, language=None):

notifications_utils/template.py

@@ -1,47 +1,45 @@
 import math
 import sys
-from os import path
 from datetime import datetime
+from html import unescape
+from os import path
 
 from jinja2 import Environment, FileSystemLoader
 from markupsafe import Markup
-from html import unescape
 
 from notifications_utils import SMS_CHAR_COUNT_LIMIT
 from notifications_utils.columns import Columns
 from notifications_utils.field import Field
 from notifications_utils.formatters import (
+    add_prefix,
+    add_trailing_newline,
+    autolink_sms, escape_html,
     insert_action_link,
-    unlink_govuk_escaped,
+    make_quotes_smart,
     nl2br,
     nl2li,
-    add_prefix,
-    autolink_sms,
+    normalise_newlines,
+    normalise_whitespace,
     notify_email_markdown,
     notify_email_preheader_markdown,
-    notify_plain_text_email_markdown,
     notify_letter_preview_markdown,
+    notify_plain_text_email_markdown,
     remove_empty_lines,
-    sms_encode,
-    escape_html,
-    strip_dvla_markup,
-    strip_pipes,
+    remove_smart_quotes_from_email_addresses,
     remove_whitespace_before_punctuation,
-    make_quotes_smart,
     replace_hyphens_with_en_dashes,
     replace_hyphens_with_non_breaking_hyphens,
-    tweak_dvla_list_markup,
+    replace_symbols_with_placeholder_parens,
+    sms_encode, strip_dvla_markup,
     strip_leading_whitespace,
-    add_trailing_newline,
-    normalise_newlines,
-    normalise_whitespace,
-    remove_smart_quotes_from_email_addresses,
+    strip_pipes,
+    strip_parentheses_in_link_placeholders,
     strip_unsupported_characters,
-)
+    tweak_dvla_list_markup,
+    unlink_govuk_escaped)
+from notifications_utils.sanitise_text import SanitiseSMS
 from notifications_utils.take import Take
 from notifications_utils.template_change import TemplateChange
-from notifications_utils.sanitise_text import SanitiseSMS
-
 
 template_env = Environment(loader=FileSystemLoader(
     path.join(
@@ -704,8 +702,14 @@ def get_html_email_body(
         strip_unsupported_characters
     ).then(
         add_trailing_newline
+    ).then(
+        # before converting to markdown, strip out the "(())" for placeholders (preview mode or test emails)
+        strip_parentheses_in_link_placeholders
     ).then(
         notify_email_markdown
+    ).then(
+        # after converting to html link, replace !!foo## with ((foo))
+        replace_symbols_with_placeholder_parens
     ).then(
         do_nice_typography
     ).then(

notifications_utils/version.py

@@ -1 +1 @@
-__version__ = '2.2.1'
+__version__ = '2.2.2'

tests/test_field_html_handling.py

@@ -57,7 +57,16 @@
         'string & entity',
         'string & entity',
     ),
-])
+],
+    ids=[
+        'string with html',
+        'string with html inside placeholder',
+        'string with placeholder',
+        'string with html inside conditional placeholder',
+        'string with conditional placeholder with html after condition',
+        'string with special character &',
+]
+)
 def test_field_handles_html(content, values, expected_stripped, expected_escaped, expected_passthrough):
     assert str(Field(content, values)) == expected_stripped
     assert str(Field(content, values, html='strip')) == expected_stripped

tests/test_template_types.py

@@ -177,10 +177,78 @@ def test_pass_through_renderer():
         'two of the same action link',
     ]
 )
-def test_get_html_email_body(content, values, expected):
+def test_get_html_email_body_with_action_links(content, values, expected):
     assert get_html_email_body(content, values) == expected
 
 
+@pytest.mark.parametrize(
+    'content, expected',
+    [
+        (
+            'normal placeholder formatting: ((foo))',
+            (
+                f'<p style="{PARAGRAPH_STYLE}">normal placeholder formatting: <span class=\'placeholder\'><mark>((foo))'
+                '</mark></span></p>'
+            ),
+        ),
+        (
+            'regular markdown link: [link text](#)',
+            (
+                f'<p style="{PARAGRAPH_STYLE}">regular markdown link: '
+                f'<a style="{LINK_STYLE}" href="#" target="_blank">link text</a></p>'
+            ),
+        ),
+        (
+            'placeholder in link text, without placeholder in link: [link ((foo))](https://test.com/)',
+            (
+                f'<p style="{PARAGRAPH_STYLE}">placeholder in link text, without placeholder in link: '
+                f'<a style="{LINK_STYLE}" href="https://test.com/" target="_blank">link '
+                '<span class=\'placeholder\'><mark>((foo))</mark></span></a></p>'
+            ),
+        ),
+        (
+            'no format within link, placeholder at end: [link text](https://test.com/((foo)))',
+            (
+                f'<p style="{PARAGRAPH_STYLE}">no format within link, placeholder at end: '
+                f'<a style="{LINK_STYLE}" href="https://test.com/((foo))" target="_blank">link text</a></p>'
+            )
+        ),
+        (
+            'no format within link, placeholder in middle: [link text](https://test.com/((foo))?xyz=123)',
+            (
+                f'<p style="{PARAGRAPH_STYLE}">no format within link, placeholder in middle: '
+                f'<a style="{LINK_STYLE}" href="https://test.com/((foo))?xyz=123" target="_blank">link text</a></p>'
+            )
+        ),
+        (
+            'no format in link, with only placeholder: [link text](((foo)))',
+            (
+                f'<p style="{PARAGRAPH_STYLE}">no format in link, with only placeholder: '
+                f'<a style="{LINK_STYLE}" href="((foo))" target="_blank">link text</a></p>'
+            )
+        ),
+        (
+            'no format within link, multiple placeholders: [link text](https://test.com/((foo))?xyz=((bar)))',
+            (
+                f'<p style="{PARAGRAPH_STYLE}">no format within link, multiple placeholders: '
+                f'<a style="{LINK_STYLE}" href="https://test.com/((foo))?xyz=((bar))" target="_blank">link text</a></p>'
+            )
+        ),
+    ],
+    ids=[
+        'formatting with placeholder',
+        'no formatting with only markdown link',
+        'formatting with placeholder in markdown link text',
+        'formatting with placeholder in markdown link url',
+        'formatting with placeholder in markdown link url and text around placeholder',
+        'formatting when placeholder is markdown link url',
+        'formatting with multiple placeholders in markdown link'
+    ]
+)
+def test_get_html_email_body_preview_with_placeholder_in_markdown_link(content, expected):
+    assert get_html_email_body(content, template_values={}, preview_mode=True) == expected
+
+
 def test_html_email_inserts_body():
     assert 'the &lt;em&gt;quick&lt;/em&gt; brown fox' in str(HTMLEmailTemplate(
         {'content': 'the <em>quick</em> brown fox', 'subject': ''}