squash 169 - corrected list css attribute value that caused unordered…

… lists to display as ordered; added more tests

notifications_utils/formatters.py

@@ -23,7 +23,7 @@
 LIST_ITEM_STYLE = 'Margin: 5px 0 5px; padding: 0 0 0 5px; font-size: 16px; line-height: 25px; color: #323A45;'
 PARAGRAPH_STYLE = 'Margin: 0 0 20px 0; font-size: 16px; line-height: 25px; color: #323A45;'
 THEMATIC_BREAK_STYLE = 'border: 0; height: 1px; background: #BFC1C3; Margin: 30px 0 30px 0;'
-UNORDERED_LIST_STYLE = 'Margin: 0 0 0 20px; padding: 0 0 20px 0; list-style-type: disk; ' \
+UNORDERED_LIST_STYLE = 'Margin: 0 0 0 20px; padding: 0 0 20px 0; list-style-type: disc; ' \
                        'font-family: Helvetica, Arial, sans-serif;'
 
 OBSCURE_WHITESPACE = (
@@ -346,13 +346,17 @@ def insert_list_spaces(md: str) -> str:
     """
     Proper markdown for lists has a space after the number or bullet.  This is a preprocessing step to insert
     any missing spaces in lists.  This preprocessing should take place before any manipulation by Mistune.
+
+    The regular expression for unordered lists replaces the bullet with the minus, which Mistune handles.
+    This is necessary because Utils allows the non-standard literal • in markdown to denote an unordered list.
+    Performing this substitution avoids having to write custom parsing logic for Mistune.
     """
 
     # Ordered lists
-    md = re.sub(r'''^(\s*)(\d+\.)''', r'''\1\2 ''', md, flags=re.M)
+    md = re.sub(r'''^(\s*)(\d+\.)(?=\S)''', r'''\1\2 ''', md, flags=re.M)
 
     # Unordered lists
-    return re.sub(r'''^(\s*)(\*|-|\+|•)(?!\2)''', r'''\1- ''', md, flags=re.M)
+    return re.sub(r'''^(\s*)(\*|-|\+|•)(?!\2)(\s*)''', r'''\1- ''', md, flags=re.M)
 
 
 def strip_parentheses_in_link_placeholders(value: str) -> str:

tests/test_formatters.py

@@ -2,23 +2,24 @@
 from markupsafe import Markup
 
 from notifications_utils.formatters import (
+    escape_html,
+    formatted_list,
+    insert_list_spaces,
+    make_quotes_smart,
+    nl2li,
+    normalise_whitespace,
     notify_html_markdown,
     notify_markdown,
-    sms_encode,
-    formatted_list,
-    strip_dvla_markup,
-    strip_pipes,
-    escape_html,
+    remove_smart_quotes_from_email_addresses,
     remove_whitespace_before_punctuation,
-    make_quotes_smart,
     replace_hyphens_with_en_dashes,
-    tweak_dvla_list_markup,
-    nl2li,
-    strip_whitespace,
+    sms_encode,
     strip_and_remove_obscure_whitespace,
-    remove_smart_quotes_from_email_addresses,
+    strip_dvla_markup,
+    strip_pipes,
     strip_unsupported_characters,
-    normalise_whitespace,
+    strip_whitespace,
+    tweak_dvla_list_markup,
 )
 from notifications_utils.template import (
     HTMLEmailTemplate,
@@ -421,7 +422,7 @@ def test_ordered_list():
                 '\n* three'
             ),
             (
-                '<ul role="presentation" style="Margin: 0 0 0 20px; padding: 0 0 20px 0; list-style-type: disk; '
+                '<ul role="presentation" style="Margin: 0 0 0 20px; padding: 0 0 20px 0; list-style-type: disc; '
                 'font-family: Helvetica, Arial, sans-serif;">\n'
                 '<li style="Margin: 5px 0 5px; padding: 0 0 0 5px; font-size: 16px; '
                 'line-height: 25px; color: #323A45;">'
@@ -430,7 +431,7 @@ def test_ordered_list():
                 '</ul>\n'
                 '<p style="Margin: 0 0 20px 0; font-size: 16px; line-height: 25px; color: #323A45;">nested 1</p>\n'
                 '<p style="Margin: 0 0 20px 0; font-size: 16px; line-height: 25px; color: #323A45;">nested 2</p>\n'
-                '<ul role="presentation" style="Margin: 0 0 0 20px; padding: 0 0 20px 0; list-style-type: disk; '
+                '<ul role="presentation" style="Margin: 0 0 0 20px; padding: 0 0 20px 0; list-style-type: disc; '
                 'font-family: Helvetica, Arial, sans-serif;">\n'
                 '<li style="Margin: 5px 0 5px; padding: 0 0 0 5px; font-size: 16px; '
                 'line-height: 25px; color: #323A45;">two</li>\n'
@@ -469,13 +470,8 @@ def test_paragraph_in_list_has_no_linebreak(test_text, expected):
             '+ two\n'
             '+ three\n'
         ),
-        pytest.param((  # bullet as bullet - This is non-standard.
-            '• one\n'
-            '• two\n'
-            '• three\n'
-        ), marks=pytest.mark.xfail(strict=False)),
     ),
-    ids=['two_spaces', 'tab', 'dash_as_bullet', 'plus_as_bullet', 'bullet_as_bullet']
+    ids=['two_spaces', 'tab', 'dash_as_bullet', 'plus_as_bullet']
 )
 @pytest.mark.parametrize(
     'markdown_function, expected',
@@ -484,7 +480,7 @@ def test_paragraph_in_list_has_no_linebreak(test_text, expected):
             notify_html_markdown,
             (
                 '<ul role="presentation" style="Margin: 0 0 0 20px; padding: 0 0 20px 0; '
-                'list-style-type: disk; '
+                'list-style-type: disc; '
                 'font-family: Helvetica, Arial, sans-serif;">\n'
                 '<li style="Margin: 5px 0 5px; padding: 0 0 0 5px; font-size: 16px; '
                 'line-height: 25px; color: #323A45;">'
@@ -993,3 +989,63 @@ def test_strip_unsupported_characters():
 
 def test_normalise_whitespace():
     assert normalise_whitespace('\u200C Your tax   is\ndue\n\n') == 'Your tax is due'
+
+
+@pytest.mark.parametrize(
+    'actual, expected',
+    [
+        (
+            '1.one\n2.two\n3.three',
+            '1. one\n2. two\n3. three',
+        ),
+        (
+            '-one\n   -two\n-three',
+            '- one\n   - two\n- three',
+        ),
+        (
+            '+one\n   +two\n+three',
+            '- one\n   - two\n- three',
+        ),
+        (
+            '*one\n   *two\n*three',
+            '- one\n   - two\n- three',
+        ),
+        (
+            '•one\n   •two\n•three',
+            '- one\n   - two\n- three',
+        ),
+        # Next 2 tests: Shouldn't misinterpret a thematic break as a list item
+        (
+            '***one\n   *two\n*three',
+            '***one\n   - two\n- three',
+        ),
+        (
+            '-one\n   ---two\n-three',
+            '- one\n   ---two\n- three',
+        ),
+        (
+            '# This is Heading 1\n'
+            '## This is heading 2\n'
+            '### This is heading 3\n'
+            '\n'
+            '__This has been emboldened__\n'
+            '\n'
+            '- this is a bullet list\n'
+            '- list list list\n'
+            '\n'
+            'Testing personalisation, ((name)).\n',
+            '# This is Heading 1\n'
+            '## This is heading 2\n'
+            '### This is heading 3\n'
+            '\n'
+            '__This has been emboldened__\n'
+            '\n'
+            '- this is a bullet list\n'
+            '- list list list\n'
+            '\n'
+            'Testing personalisation, ((name)).\n',
+        ),
+    ]
+)
+def test_insert_list_spaces(actual, expected):
+    assert insert_list_spaces(actual) == expected

tests/test_template_types.py

@@ -1584,6 +1584,7 @@ def test_ordered_list_without_spaces(template_type, expected):
         assert expected in str(template_type({'content': content, 'subject': ''}))
 
 
+@pytest.mark.parametrize('with_spaces', [True, False])
 @pytest.mark.parametrize(
     'template_type, expected',
     [
@@ -1599,12 +1600,13 @@ def test_ordered_list_without_spaces(template_type, expected):
     ]
 )
 @pytest.mark.parametrize('bullet', ['*', '-', '+', '•'])
-def test_unordered_list_without_spaces(bullet, template_type, expected):
+def test_unordered_list(bullet, template_type, expected, with_spaces):
     """
     Proper markdown for unordered lists has a space after the bullet.
     """
 
-    content = f'{bullet}one\n{bullet}two\n{bullet}three\n'
+    space = ' ' if with_spaces else ''
+    content = f'{bullet}{space}one\n{bullet}{space}two\n{bullet}{space}three\n'
 
     if isinstance(template_type, PlainTextEmailTemplate):
         assert str(template_type({'content': content, 'subject': ''})) == expected