#199 Markdown specification (part 2 of 2) #214
Conversation
notifications_utils/template2.py
Outdated
| Substitute personalization values into markdown, and return the markdown as HTML or plain text. | ||
| """ | ||
|
|
||
| # TODO - Perform substitutions in the markdown. Raise ValueError for missing fields. |
There was a problem hiding this comment.
Do we need a ticket number on this TODO
notifications_utils/template2.py
Outdated
| if html_content is None and plain_text_content is None: | ||
| raise ValueError('You must supply one of these parameters.') | ||
|
|
||
| # TODO - Perform substitutions in the subject. Raise ValueError for missing fields. |
There was a problem hiding this comment.
More TODOs - Do we need a ticket number
notifications_utils/template2.py
Outdated
| # TODO - Perform substitutions in the markdown. Raise ValueError for missing fields. | ||
|
|
||
| if as_html: | ||
| # TODO - pass the markdown to the HTML renderer | ||
| pass | ||
| else: | ||
| # TODO - pass the markdown to the plain text renderer | ||
| pass |
There was a problem hiding this comment.
TODOs require ticket numbers, please.
notifications_utils/template2.py
Outdated
| raise NotImplementedError | ||
|
|
||
|
|
||
| # TODO - The signature and return type might change after #213, during integration with notifcation-api. |
| """ | ||
| In addition to the content body, e-mail notifications might have personalization values in the | ||
| subject line, and the content body might be plugged into a Jinja2 template. | ||
|
|
||
| The two "content" parameters generally are the output of render_notify_markdown (above). | ||
|
|
||
| returns: A 2-tuple in which the first value is the full HTML e-mail; the second, the plain text e-mail. |
There was a problem hiding this comment.
I think our current docstring format is description, args, returns?
There was a problem hiding this comment.
I think that's accurate for ENP, but I was not attempting to emulate that here. It's more of a "note to self" at this point. #215 will revisit this function.
notifications_utils/template2.py
Outdated
| # TODO - Perform substitutions in the subject. Raise ValueError for missing fields. | ||
| # TODO - Jinja2 template substitution |
| @@ -0,0 +1,25 @@ | |||
| # Links With Placeholders | |||
There was a problem hiding this comment.
I don't see that all the use cases from the template QA All the Formatting are tested here. Please include them. The below should e enough information for you to go into Portal and edit the template to see the markdown. Below is what we get back from the templates routes for the placeholder in links section.
DEV VANotify Template ID "86820037-1374-4072-9f9f-e74f235170a1"
Fixed Cases:\n1. placeholder at end of [markdown link](https://test.com/((foo)))\n2. placeholder at start of [markdown link](((foo))test.com)\n3. placeholder in middle of [markdown link](https://((foo))-test.com/)\n4. placeholder as [markdown link](((foo)))\n5. multiple placeholders in [markdown link](https://((foo))-test.com?x=((foo)))\n\nEdge Cases: (less likely to happen, but working as expected)\n1. placeholder within markdown link and markdown text [markdown ((foo)) link](https://test.com/((foo)))\n2. multiple placeholders in as query params [markdown link](https://((foo))-test.com?x=((foo))&y=((foo)))\n\n---\n\nA There was a problem hiding this comment.
These are in addition to what is already tested via tests/test_files/markdown/links.md (#198). Placeholder tests focus specifically on what could go wrong during substitution. In the case of links, it's spaces in the URL. For block quotes, it's the insertion of lists.
Description
These changes add more files to the tests/test_files/ subfolders to codify the expected behavior when rendering markdown as HTML or plain text, and they implement unit tests for all the test_files/ files. The new test files focus on personalization substitutions into links and block quotes, including the substitution of lists into the latter.
Substitutions into URLs of text containing spaces should be URL-safe encoded. The goal isn't that the link is valid (that's on the user); it's that the visual presentation is correct.
The unit tests include a simple happy path test and a test for missing personalization fields, both using markdown for a paragraph. I did not exhaustively test placeholders in all the various markdown permutations we might encounter because I don't think that would add value. When the tests I actually created for the hard cases pass, I am confident the functionality will work for the more straight-forward cases.
You might note that the 3 test classes I created are repetitive. Each class tests one input markdown file. With additional parametrization, I could roll all of these classes into a single test, but I rejected that implementation in favor of only loading the input files once, via a class-scoped fixture.
issue #199
How Has This Been Tested?
I ran the unit tests. There isn't more to do for this ticket. The new tests are all skipped, the associated features are not fully implemented, and there are no changes required in notification-api.
Checklist