Support HTML content within templates #4502

theseanything · 2020-05-04T15:48:35Z

Here we aim to add support for more content types within named render_content_for blocks. Previously it wasn't possible to specify the content type within the template. e.g content_for :body is always rendered as Govspeak.

This PR adds the format option to render_content_for method within templates.

<% render_content_for :body, format: html %>
...content...
<% end %>

This new option supports :govspeak, :text and :html. Named blocks (e.g. :title, :body etc) have individual defaults, which are outlined in the docs. Otherwise all blocks default to :govspeak.

To enable this the following changes have been made:

Removed the ability to show unprocessed govspeak markdown by appending .txt on URLs. This was a debugging feature, however cannot be supported as content is pre-processed into the required format before being added to the main view. It also seemed to add unnecessary complexity when the source code is available to read.
Mandatory error_ prefix for error message names. This is to allow a default format of :text to apply to that error message content.

kevindew

Great job Sean, looks like this opens up lots of opportunities to give Smart Answers a bit more polish and be more flexible for future endeavours.

A few things come to mind:

We've got a whole ton of files with govspeak.erb extensions that can now be mixed media, shall we change all of those to .erb
There is this somewhat quirky feature in Smart Answers where it takes the text of answers and puts them into the content item as hidden search terms (For example, the details->hidden_search_terms array in: https://www.gov.uk/api/content/business-coronavirus-support-finder) I'm not sure how that works and/or if it uses the text handling
Another quirky thing we've got is the MarriageAbroadOutcomeFlattener, which I believe Platform Health may use, I don't know if that'll be negatively impacted at all? It's super weird so would be great to remove it

app/presenters/api_presenter.rb

kevindew · 2020-05-05T14:41:37Z

lib/smart_answer/erb_renderer.rb

@@ -14,22 +14,22 @@ def initialize(template_directory:, template_name:, locals: {}, helpers: [])
      @template_directory = template_directory
      @template_name = template_name
      @locals = locals
-      lookup_context = ActionView::LookupContext.new([@template_directory, FlowRegistry.instance.load_path])
+      default_view_paths = ActionController::Base.view_paths.paths.map(&:to_s)


this looks familiar 😉 - I do wonder if there's a less cryptic way to do this.

Ahah yup, definitely pinched that. Had a look and can't see an obvious nicer solution. Essentially what we are trying to do is get the govuk_publishing_components views in the view path (which is system dependent). So could do something like:

lookup_context = ActionView::LookupContext.new([ @template_directory, FlowRegistry.instance.load_path, File.expand_path("app/views", GovukPublishingComponents::Config.gem_directory), ])

But felt like this is making assumptions about where the views live inside GovukPublishingComponents

kevindew · 2020-05-05T14:43:23Z

lib/smart_answer/helpers/format_capture_helper.rb

@@ -0,0 +1,64 @@
+module SmartAnswer
+  module Helpers::FormatCaptureHelper


This seems a slightly strange namespace for this to be - as this is a component of the ErbRenderer it might make sense under that. Under helpers gives me the impression this is shared around whereas this is only used in one context.

Ah yeah good point! I'll try and re-organise this better - I think I was just trying to the code in to another file, because the erb_renderer.rb was getting super long.

I've moved them under the ErbRenderer namespace.

lib/smart_answer/helpers/format_capture_helper.rb

kevindew · 2020-05-05T14:48:39Z

lib/smart_answer/helpers/format_capture_helper.rb

+      text: [/^title$/, /^meta_description$/, /^hint$/, /^label$/, /^suffix_label$/, /^error_*./],
+    }.freeze
+
+    def content_for(name, content = nil, options = {}, &block)


I wondered whether it'd be good to rename this so that it didn't clobber the existing content_for function that ActiveView provides. Maybe it's too much of a pain in this repo at this point but it feels like we're breaking the L in SOLID that the subclassing of this function stops the original use.

Yup agree, I was trying limit breaking changes how existing templates are currently written, but probably safer to give it a new name and not try to deal with Rails internals.

test/unit/format_capture_helper_test.rb

kevindew · 2020-05-05T15:10:31Z

test/unit/format_capture_helper_test.rb

+
+      should "render govspeak by default" do
+        assert_captured_content("content", nil, "<g><p>content</p></g>")
+      end


Might want to also make clear that it's wrapped in govspeak component.

Hopefully the larger refactor of the tests makes this clear now?

app/views/smart_answers/question.html.erb

test/unit/format_capture_helper_test.rb

doc/smart-answers/erb-templates/question-templates.md

benthorner

Looks like I've been beaten to the review here! I do like the end result - it's neat ✏️. I'm content with the overall approach, and we should just address a few things before this goes in.

doc/smart-answers/erb-templates.md

test/unit/format_capture_helper_test.rb

lib/smart_answer/helpers/format_capture_helper.rb

lib/smart_answer/erb_renderer.rb

lib/smart_answer/helpers/format_capture_helper.rb

lib/smart_answer/erb_renderer.rb

theseanything · 2020-05-06T19:19:50Z

@kevindew @benthorner Thanks for the review and all the comments! Appreciate this becoming quite large and there's a lot going on - if its helpful, I can try split off smaller PRs? (The commits may be easier to follow)

We've got a whole ton of files with govspeak.erb extensions that can now be mixed media, shall we change all of those to .erb

Yup - happy for me to change that in a separate PR?

There is this somewhat quirky feature in Smart Answers where it takes the text of answers and puts them into the content item as hidden search terms (For example, the details->hidden_search_terms array in: https://www.gov.uk/api/content/business-coronavirus-support-finder) I'm not sure how that works and/or if it uses the text handling

We should be good! The hidden_search_terms array is generated here - looks like it strips out HTML and is getting strings from the Outcome and Question Presenters, which were always rendered Govspeak anyway.

Another quirky thing we've got is the MarriageAbroadOutcomeFlattener, which I believe Platform Health may use, I don't know if that'll be negatively impacted at all? It's super weird so would be great to remove it

Should be good too! - it just generates partials from a yaml file, but the views that use them e.g here them has been updated. The templates from which the partials are generated in the MarriageAbroadOutcomeFlattener aren't effected.

This feature was used for debugging by allowing a user to append .txt to a url to show the govspeak unrendered. This adds complexity to the view rendering logic with little value as a user can reference the source code instead.

This parameter provided the option to retrieve content without processing it as govspeak i.e `html: false` prevents the block being processed as govspeak and returns plain text. This is no longer needed as it was used for a debugging feature to user unprocessed govspeak, which was removed in earlier commits.

This prefix's error messages with `error_` so they conform to conventions outlined in the documentation.

The FormatCaptureHelper was added to render content_for blocks within smart answer templates. Previously it was not possible to specify the content type for particular sections within the template, as they were all post-processed in the same way. e.g. content for the body was always passed through the Govspeak processor before being rendered in the main views (result, question, start). Therefore there was no way to specify a content format within the node template. e.g. html content for the body, not processed by govspeak. This helper enables the option to specify a content format with the content_for function within the node template. To enable this we are also expanding the view paths of ErbRenderer to include those from Rail's ActionController::Base. This is because we need access GovukPublishingComponents to fully render govspeak.

As rendering of the content_for block is done at view render time. There is no need to have individual methods to access types of content from the ErbRenderer. Access can be simplified to simply delegating to the ActiveView::Base#content_for method.

Govspeak is rendered and wrapped with the govspeak component when the content_for is called in the template. Therefore there are redundant wrapping of govspeak within the main views (i.e. results, questions and landing views)

This adds the :html format for content_for block. It should render the block as is.

For consistency wih FormatCaptureHelper, this code was moved to a seperated file under the helpers folder.

The FormatCaptureHelper and QuestionOptionHelper were under the general helper module namespace. To make it more clear their purpose, they have been moved under the ErbRenderer namespace.

kevindew

Wowsers - this is a huge PR (> 900 files 😵). I've only really given a lot of them a scroll through.

I've put just a minor comment on that can be fixed without a re-review.

I do find myself a bit baffled by the render_content_for method, the naming seems a bit confusing outside of govspeak. Though I understand this naming has been what has dragged on this PR and I don't want to block your team over it. What's next for the naming discussion once this is merged?

lib/smart_answer/erb_renderer/format_capture_helper.rb

lib/smart_answer/erb_renderer/question_options_helper.rb

Previously this method was intented to override the `content_for` within ActiveView::Helpers:CaptureHelper. This changed the expected behaviour of content_for within template and could lead to confusion. It also put dependency on undocumented behaviour of the content_for within Rails, which could be fragile and confusing to support in the future. This change requires an update to all templates to use the renamed method.

As this option is only used by render_content_for, it is removed before being passed on to content_for to prevent any potential side effects.

No longer used.

Previously render_content_for marked strings of text format as html_safe. However this lead to the possibility of character's not being properly escaped. e.g. you shouldn't be able to write HTML in the text field.

theseanything · 2020-05-13T10:18:56Z

Thanks @kevindew!

Wowsers - this is a huge PR (> 900 files 😵). I've only really given a lot of them a scroll through.

Yeah sorry, most of its the renaming from content_for.

I do find myself a bit baffled by the render_content_for method, the naming seems a bit confusing outside of govspeak. Though I understand this naming has been what has dragged on this PR and I don't want to block your team over it. What's next for the naming discussion once this is merged?

Yup, so I've opened issue #4510 to continue discussion. I was initial hesitant to merge this, but think it's fine given the previous behaviour had the same issues (i.e. content_for implicitly rendered Govspeak for some sections and plaintext for others.) But there is definitely room for improvement on the syntax for these templates.

kevindew · 2020-05-13T11:39:06Z

Great stuff - looks like this is good to merge.

bevanloon temporarily deployed to smart-answer-html-templ-3bjbmf May 4, 2020 15:48 Inactive

theseanything force-pushed the html-templates branch from 2efbc0e to c2856c2 Compare May 4, 2020 17:30

bevanloon temporarily deployed to smart-answer-html-templ-3bjbmf May 4, 2020 17:30 Inactive

bevanloon temporarily deployed to smart-answer-html-templ-3bjbmf May 5, 2020 10:52 Inactive

theseanything force-pushed the html-templates branch from 9470d4a to b21e700 Compare May 5, 2020 11:01

bevanloon temporarily deployed to smart-answer-html-templ-3bjbmf May 5, 2020 11:01 Inactive

theseanything force-pushed the html-templates branch from b21e700 to 389e14d Compare May 5, 2020 11:02

bevanloon temporarily deployed to smart-answer-html-templ-3bjbmf May 5, 2020 11:02 Inactive

bevanloon temporarily deployed to smart-answer-html-templ-3bjbmf May 5, 2020 11:09 Inactive

bevanloon temporarily deployed to smart-answer-html-templ-3bjbmf May 5, 2020 12:18 Inactive

theseanything marked this pull request as ready for review May 5, 2020 12:20

theseanything requested a review from kevindew May 5, 2020 12:53

kevindew reviewed May 5, 2020

View reviewed changes

benthorner reviewed May 5, 2020

View reviewed changes

bevanloon temporarily deployed to smart-answer-html-templ-3bjbmf May 6, 2020 12:27 Inactive

bevanloon temporarily deployed to smart-answer-html-templ-3bjbmf May 6, 2020 13:27 Inactive

bevanloon temporarily deployed to smart-answer-html-templ-3bjbmf May 6, 2020 13:34 Inactive

bevanloon temporarily deployed to smart-answer-html-templ-3bjbmf May 6, 2020 15:34 Inactive

theseanything force-pushed the html-templates branch from 6018a6d to d063a43 Compare May 6, 2020 16:38

bevanloon temporarily deployed to smart-answer-html-templ-3bjbmf May 6, 2020 16:38 Inactive

theseanything force-pushed the html-templates branch from d063a43 to a3608b2 Compare May 6, 2020 18:40

bevanloon temporarily deployed to smart-answer-html-templ-3bjbmf May 6, 2020 18:40 Inactive

theseanything force-pushed the html-templates branch from a3608b2 to a063b42 Compare May 7, 2020 10:24

bevanloon temporarily deployed to smart-answer-html-templ-3bjbmf May 7, 2020 10:24 Inactive

theseanything force-pushed the html-templates branch from a063b42 to 9c16e5a Compare May 7, 2020 10:34

bevanloon temporarily deployed to smart-answer-html-templ-3bjbmf May 7, 2020 10:34 Inactive

theseanything force-pushed the html-templates branch from 9c16e5a to 46ae163 Compare May 7, 2020 11:23

bevanloon temporarily deployed to smart-answer-html-templ-3bjbmf May 7, 2020 11:23 Inactive

theseanything mentioned this pull request May 7, 2020

Implicit defaults for rendering formats in node .erb templates #4510

Closed

theseanything force-pushed the html-templates branch from 46ae163 to 7c19e3a Compare May 7, 2020 12:08

bevanloon temporarily deployed to smart-answer-html-templ-3bjbmf May 7, 2020 12:08 Inactive

theseanything force-pushed the html-templates branch from 7c19e3a to d63b666 Compare May 7, 2020 14:06

bevanloon temporarily deployed to smart-answer-html-templ-3bjbmf May 7, 2020 14:07 Inactive

theseanything added 10 commits May 7, 2020 16:03

Remove ability to view un-rendered govspeak

a564e8b

This feature was used for debugging by allowing a user to append .txt to a url to show the govspeak unrendered. This adds complexity to the view rendering logic with little value as a user can reference the source code instead.

Add prefix to error messages in erb templates

7588f76

This prefix's error messages with `error_` so they conform to conventions outlined in the documentation.

Remove govspeak component wrappers from main views

d459331

Govspeak is rendered and wrapped with the govspeak component when the content_for is called in the template. Therefore there are redundant wrapping of govspeak within the main views (i.e. results, questions and landing views)

Add ability to specify HTML in templates

fb083ad

This adds the :html format for content_for block. It should render the block as is.

Update documentation for formats on content_for

6bb4159

Move QuestionOptionHelper to seperate file

fd42666

For consistency wih FormatCaptureHelper, this code was moved to a seperated file under the helpers folder.

Move ErbRenderer helpers into class namespace

c6a706f

The FormatCaptureHelper and QuestionOptionHelper were under the general helper module namespace. To make it more clear their purpose, they have been moved under the ErbRenderer namespace.

theseanything force-pushed the html-templates branch from d63b666 to d1ec375 Compare May 7, 2020 15:11

bevanloon temporarily deployed to smart-answer-html-templ-3bjbmf May 7, 2020 15:11 Inactive

theseanything requested a review from kevindew May 12, 2020 10:10

kevindew approved these changes May 12, 2020

View reviewed changes

lib/smart_answer/erb_renderer/format_capture_helper.rb Outdated Show resolved Hide resolved

lib/smart_answer/erb_renderer/format_capture_helper.rb Show resolved Hide resolved

lib/smart_answer/erb_renderer/question_options_helper.rb Show resolved Hide resolved

theseanything added 4 commits May 13, 2020 11:10

Change render_content_for to delete :format key

82f94e0

As this option is only used by render_content_for, it is removed before being passed on to content_for to prevent any potential side effects.

Remove GovspeakPresenter

478c124

No longer used.

Remove html_safe from text formated content

8849dfc

Previously render_content_for marked strings of text format as html_safe. However this lead to the possibility of character's not being properly escaped. e.g. you shouldn't be able to write HTML in the text field.

theseanything force-pushed the html-templates branch from d1ec375 to 8849dfc Compare May 13, 2020 10:10

theseanything merged commit 99118ce into master May 13, 2020

theseanything deleted the html-templates branch May 13, 2020 11:40

theseanything mentioned this pull request May 27, 2020

Add declarative smart answer helper methods #4527

Merged

kevindew mentioned this pull request May 28, 2020

label and suffix_label fields seem broken #4553

Open

		@@ -0,0 +1,64 @@
		module SmartAnswer
		module Helpers::FormatCaptureHelper

Support HTML content within templates #4502

Support HTML content within templates #4502

Uh oh!

Conversation

theseanything commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevindew left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benthorner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

theseanything commented May 6, 2020

Uh oh!

kevindew left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

theseanything commented May 13, 2020

Uh oh!

kevindew commented May 13, 2020

Uh oh!

Uh oh!

theseanything commented May 4, 2020 •

edited

Loading