Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[#131733667] Content loader to support Jinja templating #8

Merged
merged 24 commits into from
Nov 7, 2016
Merged

[#131733667] Content loader to support Jinja templating #8

merged 24 commits into from
Nov 7, 2016

Conversation

pcraig3
Copy link
Contributor

@pcraig3 pcraig3 commented Oct 31, 2016 •
edited
Loading

Note:
⚠️ This needs to come in at the same time as version 4.x.x of the frameworks repo. PR here ⚠️

What changed

Before, all values in YAML files were stored as strings and then printed out
directly by jinja in the frontend apps. Now, we're creating TemplateFields
for specific attibutes of questions and sections. TemplateFields are wrappers
around jinja Templates that are passed an initial string and then are rendered
with a context.
The result of all this is that we can pass variables to our content which
come from briefs or lots or service data or whatever else, either to be
printed as part of the content or evaluated as part of some logic operation.

These are the fields that are turned into TemplateFields when the YAML files are loaded:

ContentSection Question
name, description question, name, question_advice, hint

ContentTemplateErrors are raised if a TemplateField is accessed (ie, rendered) without
the variables it needs. Although we don't have a way to verify that every templated
field gets all of the variables it expects in every case (we would only know
this for sure in the view logic in the frontend apps), we have tests to make sure
HTML and jinja logic is escaped before it is presented to the user.

Nothing that is currently working will break with this update, so the example
below shows how a previously static field can now be given a variable.

Example app change

Old

# digitalmarketplace-frameworks/frameworks/digital-outcomes-and-specialists/manifests/edit_brief_response.yml
name: Apply for this opportunity
# digitalmarketplace-supplier-frontend/app/main/views/briefs.py
def create_brief_response(brief_id):
  section = manifest.get_section('apply').filter({})
  return render_template(section=section)
<!-- digitalmarketplace-supplier-frontend/app/templates/briefs/brief_response.html -->
<h1>{{ section.name }}</h1>
<!-- Output -->
<h1>Apply for this opportunity</h1>

New

# digitalmarketplace-frameworks/frameworks/digital-outcomes-and-specialists/manifests/edit_brief_response.yml
name: Apply for ‘{{ brief.title }}’
# digitalmarketplace-supplier-frontend/app/main/views/briefs.py
def create_brief_response(brief_id):
  section = manifest.get_section('apply').filter({'brief': brief})
  return render_template(section=section)
<!-- digitalmarketplace-supplier-frontend/app/templates/briefs/brief_response.html -->
<h1>{{ section.name }}</h1>
<!-- Output -->
<h1>Apply for ‘Home Office IPT Programme Caseworking and DPM Delivery Partner’</h1>

(We have also replaced yml.load with yml.safe_load after reading the dire warning in the PyYAML documentation)

Relevant story on Pivotal

allait and others added 20 commits October 27, 2016 17:13
@allait
Write initial tests for the ContentSection.filter templating
c80d568 
Tests for fields that shouldn't be changed and a failing test for
templating section fields.
@allait
Run ContentSection.name through Jinja on .filter
622ad13 
Templates section.name using service_data as tempalte context.
@allait
Return None early if filtered section has no questions
85d0a05 
There's no reason to do any additional processing on the section once
we know it has no questions after `.filter` and therefore won't be
displayed.
@pcraig3 @allait
Run Jinja templating on section descriptions in .filter method
00dcad9 
Keep the value as `None` if it is never assigned, otherwise
run the value through jinja templating.

Renamed the internal `service_data` variable to `context` to better
reflect what is being passed into the .filter method.
@allait
Remove support for dictionaries as section description
68f33eb 
Replaces multiple descriptions and per-lot selection for section
descriptions with description templating.
@allait
Prevent access to templatable attributes before .filter was called
e4b4945 
Accessing section.name and section.description before calling `.filter`
could return the template text, which would be displayed to the user as
is. This would make catching these errors very hard, since no exception
is being raised.

To avoid this, we restrict access to templatable section attributes
using `__getattribute__`. This raises an exception each time they're
accessed on a section object that didn't have `.filter` called.
@pcraig3 @allait
Fix failing tests
c79fa3e 
Some of our tests were accessing our newly-templatable fields
before calling `filter` which now raises an Exception.
Easy fix is just calling `.filter` on the sections in the tests
affected.
@pcraig3 @allait
Test _filtered attribute is preserved
4612ecc 
While testing with the frontend apps, we found that the `_filtered`
attribute wasn't being copied correctly or set correctly when
returning a multiquestion as a section.
We've since fixed the problem, and now these tests verify that
everything works.
@pcraig3 @allait
Questions filter themselves
a643a1b 
Since we're going to be templating question fields, we're going
to need to be passing data into the questions themselves so that
they can decide what to do with it.

This commit doesn't allow any question fields to be templated, but
it *does* delegate the decision around filtering questions down
to the Question itself.  Now a section will call `.filter` on its
questions and see what comes back.
@allait
Template question fields
40d4fc8 
Similar to section attributes, run some of the question fields
through Jinja.
@allait
Move from templating fields in .filter to TemplateField
66238fd 
Adds utils.TemplateField class that parses the field value as
Jinja2 template. Creating the template from string is an expensive
operation, so we want to do this once when the content file is loaded
for the first time, which is why we're changing ContentLoader to replace
templatable fields with TemplateField objects.

TemplateField stores the original template string to make it possible to
compare two instances in tests.
@pcraig3
Use TemplateFields in ContentLoader
c311f64 
Using the `TEMPLATE_FIELDS` list in each class to decide which
fields should be TemplateFields, and then testing that they've
been set correctly.
@pcraig3
Render TemplateFields on access in ContentSection
8d1d3e3 
We used to prevent access to templatable fields before the
`/filter` method had been called (by checking the `_filtered`
flag).  Now, instead of rendering the fields when `.filter` is
called, we just capture the context and then render the fields
when the attribute is accessed.

This means that we don't need to iterate over/overwrite field
values in the `.filter` method since `__getattribute__` can
check the type when fields are accessed.
We can also avoid restricting access to templatable fields that
don't take any context variables since the templates render without
raising an error.
@allait
Render TemplateField fields in Question.__getattr__
886ea33 
Takes a similar approach to ContentQuestion, rendering templates
on attribute access.

Since Multiquestions have nested questions we need to pass context
to them the same way we do it in `section.filter`.
@allait
Add tests for TemplateField Jinja rendering
3b42b3a
@allait @pcraig3
Run template fields through Markdown first
3a34885 
At the moment, some of the content fields are processed with a
markdown filter to make writing question hints and descriptions
easier.

Since we're introducing Jinja to replay user input in question and
section data we need to move the markdown processing before Jinja,
since otherwise we might render user input as markdown.

We're only applying markdown to multiline fields since that's where
it's used by the content repo and we want to avoid wrapping single
line strings with paragraph tags.

This also introduces a number of issues with Markdown recognising
some of the characters in Jinja tags (eg '<', '*', '_') and causing
template errors by replacing them with HTML. Our main concern is that
we raise these errors early since they don't seem to prevent us from
doing common things we want to do with Jinja.
@pcraig3
Return Markup string from TemplateField.render
4378ecd 
Any HTML tags that come out of our TemplateFields will be
escaped in the actual webpage if we don't wrap them in Markup.
@pcraig3
Test that XSS vulnerabilites are mitigated
a643c9a 
To prevent cross-site scripting attacks, we want to make sure that
HTML or jinja templating that would be passed in through the
context don't end up as valid HTML once the template is rendered.

Tests make sure that HTML tags are escaped and that jinja tags are
printed literally instead of being interpreted.
@pcraig3
Use .safe_load for yml stuff
474d9ad 
[Warning: It is not safe to call yaml.load with any data received from an untrusted source!](http://pyyaml.org/wiki/PyYAMLDocumentation#LoadingYAML)
@pcraig3
Bump version to 2.0.0
6c85228 
Also updated CHANGELOG with summary of changes
@pcraig3 pcraig3 changed the title Content templating [#131733667] Content templating Oct 31, 2016
@pcraig3 pcraig3 changed the title [#131733667] Content templating [#131733667] Content loader to support Jinja templating Oct 31, 2016
@pcraig3
Return question if found
28f0b12 
Previous logic would only return a question if it was not previously
loaded.  Meant it would return the question we wanted once
but then never again.
@pcraig3
Give _context attribute to QuestionSummary
fc036ca 
QuestionSummaries were losing their _context and thereby failing
to render after getting caught in the `__getattr__` check.

Also reordered the `__getattr__` method so that it's easier to
read.
@pcraig3 pcraig3 mentioned this pull request Nov 1, 2016
Merged
@allait
Template all fields in content messages
c20db72 
In order to remove unsafe uses of `| markdown` filters we need to
add TemplateField support to content messages. Unfortunately,
unlike sections and questions messages don't have a common list of
templatable or HTML-safe attributes. In the existing messages we
have markdown fields in nested dictionaries and HTML tags within
lists of strings, which means that a predefined list of templatable
fields wouldn't really work.

Instead, we convert all fields into TemplateFields: string fields,
nested dictionary values and strings in list attributes.

This also removes support for the unused sub_key arguments since
we moved away from using compound keys.
pcraig3
pcraig3 commented Nov 3, 2016
View reviewed changes
dmcontent/utils.py
elif isinstance(item, collections.Sequence):
return [template_all(i) for i in item]
elif isinstance(item, collections.Mapping):
result = {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be a dictionary comprehension? Would look a bit more like the logic of the previous conditions.

@allait allait merged commit 814c4ee into master Nov 7, 2016
@allait allait deleted the content-templating branch November 7, 2016 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pcraig3 @allait