Improve display of 404 pages #572

tilsammans · 2016-06-29T15:20:37Z

With this change, the EntriesController will now raise
ActiveRecord::RecordNotFound for all unknown formats.

This bubbles up to the ApplicationController where it is currently
dealth with in a rescue_from block. This has been changed to simply
render a 404 head response.

This frees us from trying to come up with appropriate response formats
while still giving the correct HTTP header.

I would strongly prefer to entirely ditch the rescuing from
ActiveRecord::RecordNotFound and let the host application deal with this
problem. It seems something that is very specific to the host, and not
up to Pageflow.

fixes #40

tilsammans · 2016-06-29T15:23:26Z

@tf this is just an idea I had that will deal with this error gracefully, in my opinion. By raising this error we can let other layers deal with this problem. In this case that will be the application controller, but I would actually suggest to not rescue that and let the host application deal with it.

For example, I know that the regular 404 had hard-coded German in it, which is not something easily useful for English apps. I would say that raising is the most appropriate thing to do, even if we're not strictly trying to find a non-existing ActiveRecord.

Your ideas?

When pursued the public folder and associated stylesheets should be cleaned up too.

I never could find a reference to the 500.html, it seems to be an orphan.

coveralls · 2016-06-29T15:28:04Z

Coverage decreased (-0.02%) to 97.609% when pulling b804ad3 on scrollytelling:404-improvements into 9bdf06f on codevise:master.

tf · 2016-06-30T10:19:26Z

You make some compelling points and, as #40 states, things are currently a bit broken in this area.

First, I'd suggest removing the format.any from EntriesController completely since we already are rescuing UnknownFormat in the ApplicationController. We should probably improve our response there instead of rendering the text "Not found".

Regarding removing all rescuing logic I'm skeptical. Even if Pageflow::ApplicationController inherited from ApplicationController, forcing the host application to handle all cases there seems like leaking to much implementation detail to me. But what do you think about having the install generator create public/pageflow/404.html and rendering that? That way the host application has full control of what is rendered, but does not have to care about when this happens.

Keeping files in public also makes it easy to reuse files in the Nginx config. That's where the stray 500.html comes from.

For more complex integration, I'd otherwise suggest doing this via new config options, i.e. allowing to register a lambda that handles the case where an entry is not found. That could then deal with specific needs like displaying a localized error page etc.

Thoughts?

tilsammans · 2016-06-30T11:20:34Z

Tweaking the generator is an excellent idea! This is the least surprising alternative and quite flexible too.

I might be able to start work on this next week.

coveralls · 2016-07-13T09:38:42Z

Coverage decreased (-0.01%) to 97.733% when pulling c258942 on scrollytelling:404-improvements into 2249619 on codevise:master.

houndci-bot · 2016-07-13T10:06:45Z

spec/generators/pageflow/not_found/not_found_generator_spec.rb

+      it "copies the images" do
+        run_generator
+
+        expect(file("public/pageflow/images/bg.jpg")).to exist


Prefer single-quoted strings when you don't need string interpolation or special symbols.

tilsammans · 2016-07-13T10:11:16Z

@tf I pushed some new commits.

Once people upgrade to 0.11/0.12 we would have to urge them to re-run the generator.

coveralls · 2016-07-13T10:12:51Z

Coverage decreased (-55.8%) to 41.981% when pulling ffbf3f8 on scrollytelling:404-improvements into 2249619 on codevise:master.

coveralls · 2016-07-13T10:12:57Z

Coverage decreased (-0.06%) to 97.679% when pulling ffbf3f8 on scrollytelling:404-improvements into 2249619 on codevise:master.

coveralls · 2016-07-13T10:19:48Z

Coverage decreased (-55.8%) to 41.981% when pulling 28b2d87 on scrollytelling:404-improvements into 2249619 on codevise:master.

coveralls · 2016-07-13T11:19:16Z

Coverage increased (+0.05%) to 97.792% when pulling bac0378 on scrollytelling:404-improvements into 2249619 on codevise:master.

coveralls · 2016-07-13T11:53:31Z

Coverage decreased (-0.05%) to 97.69% when pulling 88bb088 on scrollytelling:404-improvements into 2249619 on codevise:master.

tf · 2016-07-13T13:46:03Z

lib/generators/pageflow/install/install_generator.rb

        invoke 'pageflow:user'
        invoke 'pageflow:seeds'
        invoke 'pageflow:active_admin_initializer'
+        invoke 'pageflow:not_found'


I think pageflow:error_pages would be a bit more descriptive and would also allow to include files for other status codes should we ever need them.

tf · 2016-07-13T13:58:49Z

Looks really good already! And for the upgrade step running the pageflow:error_pages generator should be enough, right? We can mention that in the changelog.

houndci-bot · 2016-07-14T12:18:07Z

app/controllers/pageflow/application_controller.rb

-        format.any(:json, :css) { head :not_found }
+        format.html do
+          begin
+            render file: Rails.public_path.join('pageflow', 'error_pages', '404.html'), status: :not_found


Line is too long. [106/100]

I'm still unsure if we should have a test for this? The line itself is covered. We could add an expection to the EntriesController#show spec that some text from the generated file shows up. For the rescue clause below, I see no way to get test coverage. A test could temporarily rename the 404.html file in the dummy app. But I'm not sure it's worth it.

I think it's worth it because the head :not_found in ApplicationController is part of our code. I added 527f703 to test this. It might not look pretty but it is effective. I added the safeguard since the test could fail for other reasons and leave the dummy app in an unclean state.

An alternative could be to not bother with file-level stuff (which technically doesn't belong in a controller test anyway) but stub out the call to render and raise ActionView::MissingTemplate from the test. But then you're testing Rails framework detail which might change at some point...

Not sure what we should prefer here. But I do think a test has value.

As far as testing response content, that has little additional value to me over the status code. Also it makes the test brittle in case someone wants to put in new text in this file.

tilsammans · 2016-07-14T12:21:58Z

And for the upgrade step running the pageflow:error_pages generator should be enough, right?

Yes.

Just pushed up the new commit to move error pages into its own directory. Final review?

coveralls · 2016-07-14T12:23:38Z

Coverage decreased (-0.008%) to 97.736% when pulling c556423 on scrollytelling:404-improvements into 2249619 on codevise:master.

coveralls · 2016-07-14T12:32:13Z

Coverage decreased (-0.008%) to 97.736% when pulling efa67ba on scrollytelling:404-improvements into 2249619 on codevise:master.

tf · 2016-07-14T14:07:47Z

lib/generators/pageflow/error_pages/templates/404.html

 <html>
 <head>
  <meta charset="utf-8"/>
  <title>Beitrag nicht gefunden (404)</title>


The generated default file should probably be in english as that's also the default locale.

tf · 2016-07-14T14:09:00Z

Added two more notes. Not sure about the test issue. Please again squash to one descriptive commit when you're done.

houndci-bot · 2016-07-14T20:48:35Z

spec/controllers/pageflow/entries_controller_spec.rb


+    def without_error_pages
+      error_pages = __dir__ + "/../../dummy/rails-#{Pageflow::RailsVersion.detect}/public/pageflow/error_pages"
+      fail "Pageflow error pages not present in the dummy app. Remove `spec/dummy` to regenerate it." unless File.exists?("#{error_pages}/404.html")


Line is too long. [148/100]
Prefer single-quoted strings when you don't need string interpolation or special symbols.

coveralls · 2016-07-14T21:13:54Z

Coverage decreased (-0.01%) to 97.728% when pulling 4827847 on scrollytelling:404-improvements into 2249619 on codevise:master.

coveralls · 2016-07-14T21:14:39Z

Coverage decreased (-0.01%) to 97.728% when pulling 4827847 on scrollytelling:404-improvements into 2249619 on codevise:master.

tf · 2016-07-15T07:43:26Z

The test does not seem to work. I got suspicious when I saw that the rescue clause still has no test coverage. Running the test locally a few times reveals that rails caches rendered files. So if a spec before already rendered the 404.html, renaming it does not hurt. I'm not sure what to do. Stubbing render also feels dirty. We could take this as an excuse to make the path to the error page configurable. That way, if the host app already has a public/404.html, Pageflow could be configured to reuse that. In our test though, we could point the config option to a non-existing path and check for the head response. What do you think?

Regarding testing the response body: The current test does not really ensure the template is used. For all we know this could also be the head response. Maybe we can at least test that the response is not empty or looks like an html page.

Finally, I'm not so fond of using rand in tests. If the id really does not matter, we can just as well choose one or use a string like "not_there".

Introduce a generator that generates the static 404.html into the host application's public folder. Pageflow will try to render this page in case of a PageNotFound. When the page is absent, a :not_found 'head' response is given instead. Upgrading host applications should run the pageflow:error_pages generator to generate these files. Afterwards they can be customized at will. This change also takes out the format.any fallbacks, since these have been handled in the ApplicationController for a while anyway. Fixes #40

tilsammans · 2016-07-16T18:34:53Z

@tf I took out the test and left it at that for the moment.

Adding the config feels a little scope-creepy to me, but not a bad idea in principle. If we're doing that only to satisfy the test case, that's probably not the right reason though.

I will be on vacation for three weeks, with little access to GitHub.

tf · 2016-07-17T08:51:49Z

Agreed. Have a nice vacation.

tf changed the title ~~raise RecordNotFound instead of render 404.html~~ [WIP] Improve display of 404 pages Jun 30, 2016

houndci-bot reviewed Jul 13, 2016
View reviewed changes

tf reviewed Jul 13, 2016
View reviewed changes

houndci-bot reviewed Jul 14, 2016
View reviewed changes

tf reviewed Jul 14, 2016
View reviewed changes

houndci-bot reviewed Jul 14, 2016
View reviewed changes

tf changed the title ~~[WIP] Improve display of 404 pages~~ Improve display of 404 pages Jul 17, 2016

tf added this to the v0.11 milestone Jul 17, 2016

tf merged commit fcde7b4 into codevise:master Jul 17, 2016

Improve display of 404 pages #572

Improve display of 404 pages #572

Uh oh!

Conversation

tilsammans commented Jun 29, 2016

Uh oh!

tilsammans commented Jun 29, 2016

Uh oh!

coveralls commented Jun 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tf commented Jun 30, 2016

Uh oh!

tilsammans commented Jun 30, 2016

Uh oh!

coveralls commented Jul 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tilsammans commented Jul 13, 2016

Uh oh!

coveralls commented Jul 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tf commented Jul 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

tilsammans commented Jul 14, 2016

Uh oh!

coveralls commented Jul 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tf commented Jul 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jul 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tf commented Jul 15, 2016

Uh oh!

tilsammans commented Jul 16, 2016

Uh oh!

tf commented Jul 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

coveralls commented Jun 29, 2016 •

edited

Loading

coveralls commented Jul 13, 2016 •

edited

Loading

coveralls commented Jul 13, 2016 •

edited

Loading

coveralls commented Jul 13, 2016 •

edited

Loading

coveralls commented Jul 13, 2016 •

edited

Loading

coveralls commented Jul 13, 2016 •

edited

Loading

coveralls commented Jul 13, 2016 •

edited

Loading

tf commented Jul 13, 2016 •

edited

Loading

coveralls commented Jul 14, 2016 •

edited

Loading

coveralls commented Jul 14, 2016 •

edited

Loading

coveralls commented Jul 14, 2016 •

edited

Loading

coveralls commented Jul 14, 2016 •

edited

Loading