Allow questions to use ERB templates #2103
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
floehopper
changed the title
This was referenced Nov 17, 2015
|
This all looks good to me. |
I want to be able to enable the use of ERB templates for questions on a question-by-question basis and so I need to be able to pass in an option to the constructor for all question types.
I want to be able to switch between obtaining the error message from either the I18nRenderer or the ErbRenderer and extracting this method will make that easier.
This makes the test more robust to changes I'm about to make.
As far as I can tell, `SmartAnswer::State#error` is only ever set to be the message from an `InvalidResponse` exception [1] i.e. it will always be a `String`. Also the key is always interpolated into the full i18n String key in the renderer [2]. Thus it seems unnecessary and confusing to convert it to a symbol when calling `QuestionPresenter#error_message_for` to lookup a specific error key, but not when calling the same method to lookup the fallback error key, 'error_message'. [1]: https://github.com/alphagov/smart-answers/blob/6302a552ab01e357513cc0b60f31cb08aa1d382e/lib/smart_answer/flow.rb#L125 [2]: https://github.com/alphagov/smart-answers/blob/7e26c554be106b2888f44bf92746e5fc7f6d819f/lib/smart_answer/i18n_renderer.rb#L22
At the moment this just calls `#translate_option`, but this will make it easier to introduce the option to switch between I18n vs ERB rendering.
This makes it possible to switch between rendering questions using the `SmartAnswer::I18nRenderer` or the `SmartAnswer::ErbRenderer` on a per question basis. Initially I tried to split the `SmartAnswer::QuestionPresenterTest` into two contexts, but this didn't play well with the existing use of the `test` method (i.e. not `should`), so I plumped for a new test case in a separate file. Eventually we hope to do away with the `I18nRenderer` style and make the `ErbRenderer` style the default. When this happens, we'll be able to replace the `SmartAnswer::QuestionPresenterTest` with the new test case. Note that I decided to use a symbol to lookup error message using ErbRenderer to be consistent with the other calls to `#content_for`.
It doesn't seem sensible to use template names including a trailing question mark.
Also I want to make a generic cross-question-type change and so having this code in one place will make that easier.
After calling `Flow#use_erb_templates_for_questions`, all subsequently defined questions will have the `:use_erb_template` option set to `true`.
The vast majority of these files use hyphens instead of underscores to match their parent flow. This brings the two remaining exceptions into line.
I ran the following command:
$ rails r script/convert-question-i18-yaml-to-erb-templates.rb test/fixtures/smart_answer_flows/bridge-of-death
I ran the following command:
$ rails r script/convert-question-i18-yaml-to-erb-templates.rb test/fixtures/smart_answer_flows/checkbox-sample
I ran the following command:
$ rails r script/convert-question-i18-yaml-to-erb-templates.rb test/fixtures/smart_answer_flows/country-and-date-sample
I ran the following command:
$ rails r script/convert-question-i18-yaml-to-erb-templates.rb test/fixtures/smart_answer_flows/country-legacy-sample
I ran the following command:
$ rails r script/convert-question-i18-yaml-to-erb-templates.rb test/fixtures/smart_answer_flows/custom-errors-sample
I ran the following command:
$ rails r script/convert-question-i18-yaml-to-erb-templates.rb test/fixtures/smart_answer_flows/data-partial-sample
I ran the following command:
$ rails r script/convert-question-i18-yaml-to-erb-templates.rb test/fixtures/smart_answer_flows/postcode-sample
I ran the following command:
$ rails r script/convert-question-i18-yaml-to-erb-templates.rb test/fixtures/smart_answer_flows/precalculation-sample
I ran the following command:
$ rails r script/convert-question-i18-yaml-to-erb-templates.rb test/fixtures/smart_answer_flows/value-sample
I ran the following command:
$ rails r script/convert-question-i18-yaml-to-erb-templates.rb test/fixtures/smart_answer_flows/money-and-salary-sample
…stion ERB templates
I ran the following command:
$ rails r script/convert-question-i18-yaml-to-erb-templates.rb test/fixtures/smart-answers-controller-sample-with-date-question
I've also removed all the i18n-related code from the test.
…estion ERB templates
I ran the following command:
$ rails r script/convert-question-i18-yaml-to-erb-templates.rb test/fixtures/smart-answers-controller-sample-with-value-question
I've also removed all the i18n-related code from the test.
…estion ERB templates
I ran the following command:
$ rails r script/convert-question-i18-yaml-to-erb-templates.rb test/fixtures/smart-answers-controller-sample-with-money-question
I've also removed all the i18n-related code from the test.
…uestion ERB templates
I ran the following command:
$ rails r script/convert-question-i18-yaml-to-erb-templates.rb test/fixtures/smart-answers-controller-sample-with-salary-question
I've also removed all the i18n-related code from the test.
… to use question ERB templates
I ran the following command:
$ rails r script/convert-question-i18-yaml-to-erb-templates.rb test/fixtures/smart-answers-controller-sample-with-multiple-choice-question
I've also removed all the i18n-related code from the test.
I ran the following command:
$ rails r script/convert-question-i18-yaml-to-erb-templates.rb test/fixtures/smart-answers-controller-sample
I've also removed all the i18n-related code from the test.
There is code in place to handle question titles with interpolation, but that code was not being tested. I'm about to change this flow to use ERB templates for questions and I want decent test coverage in place so that I don't introduce any regressions.
I suspect this may have been used in the past, but it isn't being used now. Removing this will make it easier to see what's going on when I change this flow to use ERB templates for questions.
I ran the following command:
$ rails r script/convert-question-i18-yaml-to-erb-templates.rb test/fixtures/smart-answers-controller-sample
Even though this flow is used in a unit test, I elected to use the
`FixtureFlowsHelper` methods in order to setup the template directory for the
question nodes. It would probably be better to make it possible to configure
the template directory when instantiating a flow, but that's a job for another
day.
I've implemented a solution for coping with interpolating values which are not
available from the state object. The `MethodMissingHelper` is mixed into the
view context and builds an instance of `MethodMissingObject`. The latter
implements a `#to_s` method which returns the name of the method.
Now that ERB tags can be used for interpolation, it's theoretically possible to
have a chain of method calls instead of a single level which was all that was
possible with I18n interpolation. The above solution only deals with a single
level.
I've also removed all the i18n-related code from the test.
Now that ERB tags can be used for interpolation, it's possible to have a chain of method calls instead of a single level which was all that was possible with I18n interpolation. I've therefore changed the flow fixture to add test coverage for this scenario and enhanced `MethodMissingObject` so that it can handle that scenario.
I want to re-use this in `FlowRegistrationPresenter`.
I ran the following command:
$ rails r script/convert-question-i18-yaml-to-erb-templates.rb test/fixtures/flow-sample
This flow is used in two unit tests:
* `SmartAnswer::FlowRegistryTest`
This test didn't need any changes, because it wasn't making use of any of the
I18n content.
* `FlowRegistrationPresenterTest`
Even though this unit test, I elected to use the `FixtureFlowsHelper` methods in
order to setup the template directory for the question nodes. It would probably
be better to make it possible to configure the template directory when
instantiating a flow, but that's a job for another day.
Now that ERB tags can be used for interpolation, it's possible to have a chain
of method calls instead of a single level which was all that was possible with
I18n interpolation. I've therefore changed the flow fixture to add test
coverage for this scenario and re-used the same technique as for the
`GraphPresenter` to handle this scenario i.e. using `MethodMissingHelper` and
`MethodMissingObject`. Since we're generating content that is intended to be
used in a search index, in this case we don't want to display the missing
variable names; we just need to avoid raising an exception.
To avoid having to create a whole new flow for the one interpolation test,
I opted to add a question to the flow within that one test.
I've renamed the tests which talk about translations since we're now using ERB
rendering not I18n translation.
floehopper
force-pushed
the
use-erb-templates-for-questions
branch
from
9e56717
to
6994b3e
Compare
|
I've rebased this against master and force-pushed in preparation for merging. |
floehopper
added a commit
that referenced
this pull request
Nov 18, 2015
Allow questions to use ERB templates
floehopper
added a commit
that referenced
this pull request
Nov 19, 2015
In #2103 I forgot that we still need to support flows/questions with their content in i18n YAML files vs ERB templates. In this commit I've renamed the `FlowRegistrationPresenterTest` to `FlowRegistrationPresenterWithErbRendererTest` and reinstated the earlier version of `FlowRegistrationPresenterTest` to give suitable test coverage. I've also reinstated the two i18n YAML files and use an explicit call to `Flow#use_erb_templates_for_questions` in `FlowRegistrationPresenterWithErbRendererTest` to switch over to ERB rendering. Finally I've also reinstated the handling of `I18n::MissingInterpolationArgument` so that the tests all pass.
floehopper
added a commit
that referenced
this pull request
Nov 19, 2015
In #2103 I forgot that we still need to support flows/questions with their content in i18n YAML files vs ERB templates. In this commit I've renamed the `GraphPresenterTest` to `GraphPresenterWithErbRendererTest` and reinstated the earlier version of `GraphPresenterTest` to give suitable test coverage. I've also reinstated the i18n YAML file and used an explicit call to `Flow#use_erb_templates_for_questions` in `GraphPresenterWithErbRendererTest` to switch over to ERB rendering. I've avoided needing to handle `I18n::MissingInterpolationArgument` by passing in an empty `Hash` as the `state` when instantiating the `QuestionPresenter`.
floehopper
added a commit
that referenced
this pull request
Nov 19, 2015
In #2103 I forgot that we still need to support flows/questions with their content in i18n YAML files vs ERB templates. In this commit I've renamed the `FlowRegistrationPresenterTest` to `FlowRegistrationPresenterWithErbRendererTest` and reinstated the earlier version of `FlowRegistrationPresenterTest` to give suitable test coverage. I've also reinstated the two i18n YAML files and use an explicit call to `Flow#use_erb_templates_for_questions` in `FlowRegistrationPresenterWithErbRendererTest` to switch over to ERB rendering. Finally I've also reinstated the handling of `I18n::MissingInterpolationArgument` so that the tests all pass.
floehopper
added a commit
that referenced
this pull request
Nov 19, 2015
In #2103 I forgot that we still need to support flows/questions with their content in i18n YAML files vs ERB templates. In this commit I've renamed the `GraphPresenterTest` to `GraphPresenterWithErbRendererTest` and reinstated the earlier version of `GraphPresenterTest` to give suitable test coverage. I've also reinstated the i18n YAML file and used an explicit call to `Flow#use_erb_templates_for_questions` in `GraphPresenterWithErbRendererTest` to switch over to ERB rendering. I've avoided needing to handle `I18n::MissingInterpolationArgument` by passing in an empty `Hash` as the `state` when instantiating the `QuestionPresenter`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes it possible to optionally switch a question or flow to use
SmartAnswer::ErbRendererinstead of the defaultSmartAnswer::I18nRenderer.To do this for an individual question, you need to pass in
use_erb_template: truewhen declaring the question in the flow. If you callFlow#use_erb_templates_for_questionsin the flow definition, then all subsequent questions will use an ERB template.I've implemented a hacky script (
script/convert-question-i18-yaml-to-erb-templates.rb) to do most of the conversion automatically. It warns you if there are any top-level phrases in the i18n YAML file, because you'll probably need to make some manual changes to implement the same functionality in the ERB templates. It also warns you if an individual ERB template contains interpolation, because you will need to manually convert these to use ERB tags. Note that this script hasn't been exhaustively tested, so it may need tweaking to run against the published flows.I have also switched all the fixture flows over to use ERB templates as a way of trying out the new functionality. The only one I haven't changed over is
QuestionPresenterTest. This is because I've added a "parallel"QuestionPresenterWithErbRendererTestwhich exercises the same presenter but with theErbRenderer. Once we've switched all the published flows over, we will be able to remove theI18nRendererand replaceQuestionPresenterTestwithQuestionPresenterWithErbRendererTest.