Change the default error on strict html failure to warn on dev and test #194
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
mcgoooo
changed the title
Previously you had fundamentally different behaviour on dev as your do in production. It used to raise only on dev and test, and silently fall through on production. The content items on live are not always vaild in respect to multiple id's. This pull makes it warn in the console, and have the bell sound so you can render pages, while still keeping a reasonable warning in place. I Think ideally we would raise a logging error on production too, but we need to make sure the consequences of that would be ok, this may be tackled in a further pull. Making sure that valid html is coming through from govspeak, shuld ideally be tackled by the govspeak gem/publisher at the source, so they can fix the errors when they create them. The code for doing the logging in slimmer seems to be a bit hokey so i have overridden to use the default rails logger on dev and test if it is there. For some reason, it passes in a nil logger, and then also overrides that in this class, so i used a small hammer as it would involve unwinding a lot of logging behaviour, which i did try.
mcgoooo
force-pushed
the
Change-raise-to-warn-on-dev
branch
from
b75eba2
to
aef8897
Compare
|
this is a screenshot of new dev output |
fofr
reviewed
Apr 19, 2017
| @@ -24,6 +24,7 @@ Gem::Specification.new do |s| | |||
| s.add_dependency 'null_logger' | |||
| s.add_dependency 'rest-client' | |||
| s.add_dependency 'activesupport' | |||
| s.add_dependency 'colorize' | |||
There was a problem hiding this comment.
This seems like an unnecessary dependency.
There was a problem hiding this comment.
maybe, but it is good to have colored error messages, civil servants are users :D
fofr
reviewed
Apr 19, 2017
| # this all as it depends on all apps that use slimmer. | ||
| # This will make sure everything is logged correctly | ||
| # and put slimer renderings more in line with rendering | ||
| # normal templates |
There was a problem hiding this comment.
I don't understand this comment block.
There was a problem hiding this comment.
so the logger is dependent on what you pass through from the Rails.application.config.slimmer in the consuming app of slimmer
There was a problem hiding this comment.
seeing as none of the apps seem to pass it in, it is overridden in app.rb with null instance logger, so the options are either fix all the apps to get the error or use the small hammer of using the rails.logger if it is there
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.
Previously you had fundamentally different behaviour on dev as your do
in production. It used to raise only on dev and test, and silently
fall through on production. The content items on live are not
always vaild in respect to multiple id's. This pull makes it warn
in the console, and have the bell sound so you can render pages,
while still keeping a reasonable warning in place.
I Think ideally we would raise a logging error on production too,
but we need to make sure the consequences of that would be ok,
this may be tackled in a further pull. Making sure that valid html is
coming through from govspeak, shuld ideally be tackled by the govspeak
gem/publisher at the source, so they can fix the errors when they create
them.
The code for doing the logging in slimmer seems to be a bit hokey
so i have overridden to use the default rails logger on dev and test
if it is there. For some reason, it passes in a nil logger,
and then also overrides that in this class, so i used a small hammer
as it would involve unwinding a lot of logging behaviour,
which i did try.