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.

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

Allow custom 404.html on projects #5130

Merged
merged 17 commits into from Feb 6, 2019
Merged

Conversation

@humitos
Copy link
Member

@humitos humitos commented Jan 19, 2019

We had a discussion about this and we arrange on:

  1. serve 404.html from the root of the version is currently visited if exists.
  2. that 404.html page can be created manually or via the extesion sphinx-notfound-page.
  3. we won't publish docs around this just yet to test it better

NOTE: considering the status of the extension, we don't want to make it a dependency of Read the Docs for all the builds yet because there are more work to do on it: render from a template, write tests and documentation.

Once the extension gets stable, we will include it in the build process and we will be generating better 404 pages for all the project without user action. Allowing the user to customize the default page if they prefer.

I tested this locally and it worked. I had to override some settings so Django calls handler_404:

PYTHON_MEDIA = True
DEBUG = False

after running runserver you can access to any not found page and see the custom one if the project has one.

Closes #353

@humitos humitos requested a review from Jan 19, 2019

if os.path.exists(fullpath):
log.debug('Serving custom 404.html page for project: %', project.slug)
r = HttpResponse(open(fullpath).read(), content_type='text/html')
Copy link
Member Author

@humitos humitos Jan 19, 2019

I'm not sure if we want to serve this file from Django or we prefer to use the X-Accel-Redirect header here.

Copy link
Member Author

@humitos humitos Jan 19, 2019

In fact, I think this is a bug (or a potential bug). I did the same at https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/core/views/serve.py#L269 but, what happen if this file is too big, for example?

I suppose that we should rely on NGINX to serve them.

I'd like to get some opinions here.

Copy link
Member

@ericholscher ericholscher Jan 21, 2019

The main issue here is that it doesn't work locally. We should likely have it do Nginx when DEBUG=False, and serve locally when DEBUG=True.

Copy link
Member

@ericholscher ericholscher Jan 21, 2019

Actually, it should perhaps respect PYTHON_MEDIA instead of DEBUG

Copy link
Member Author

@humitos humitos Jan 31, 2019

Well, 404 handler is not called at all on DEBUG=True, so we will need DEBUG=False anyway to get at this point.

I agree that it should rely on PYTHON_MEDIA only.

Copy link
Member Author

@humitos humitos Jan 31, 2019

I'm using readthedocs.core.views.serve._serve_file that does this for us ;)

filename = filename[1:]

if version.privacy_level == PRIVATE:
symlink = PrivateSymlink(project)
Copy link
Member Author

@humitos humitos Jan 19, 2019

Do we need to do something special with private versions? I don't think so, but I'm not sure.

@@ -124,7 +129,36 @@ def server_error_404(request, exception=None, template_name='404.html'): # pyli
)
else:
return response
r = render(request, template_name)

project, path = project_and_path_from_request(request, request.get_full_path())
Copy link
Member Author

@humitos humitos Jan 19, 2019

This function comes from readthedocs.redirects.utils but we are not using it here for any redirect. This may worth a refactor since this function is called here and also inside of get_redirect_response some lines above.

r = render(request, template_name)

project, path = project_and_path_from_request(request, request.get_full_path())
version_slug = project.get_default_version()
Copy link
Member Author

@humitos humitos Jan 19, 2019

This fails when a 404 is generated from RTD itsefl because project is None, not a subdomain: http://localhost:8000/pagenotfound

Copy link
Member Author

@humitos humitos Jan 21, 2019

I'm thinking on writing a special serve_error_404 for subdomains only and use it only on urls/subdomain.py. Any other idea?

Copy link
Member

@ericholscher ericholscher Jan 21, 2019

Yea, we shouldn't replace the default 404 for RTD, just for subdomains.

docs/guides/custom-404-page.rst Outdated Show resolved Hide resolved
docs/guides/custom-404-page.rst Outdated Show resolved Hide resolved
docs/guides/custom-404-page.rst Outdated Show resolved Hide resolved

In case you want to follow the same style for the 404 page than your theme, you can either:

1. manually copy the source of any already rendered pages or,
Copy link
Member

@ericholscher ericholscher Jan 21, 2019

We shouldn't recommend this, but just point to the extension.

Copy link
Member Author

@humitos humitos Jan 31, 2019

I removed this item and the whole "Manually creation" section.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jan 29, 2019

@humitos we should get this put together and shipped.

@humitos
Copy link
Member Author

@humitos humitos commented Feb 4, 2019

We talked about installing the sphinx-notfound-page extension by default and always build a 404 page for projects we host, so they never see our own 404 Maze Found page.

@humitos
Copy link
Member Author

@humitos humitos commented Feb 4, 2019

This is an example of what is generated automatically by Read the Docs using sphinx-notfound-page.

captura de pantalla_2019-02-04_14-19-42

It keeps the theme style, also maintains the flyout menu and all the toctree links in the left (we may want to remove the later since could not be the same among versions)

It has a CSS problem that I don't know what's happening and/or how to fix it yet.

@humitos humitos force-pushed the humitos/allow-custom-404-pages branch from 48a1f55 to c25509c Feb 4, 2019
@humitos humitos mentioned this pull request Feb 4, 2019
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Feb 4, 2019

This is an example of what is generated automatically by Read the Docs using sphinx-notfound-page.

I think it's probably time to retire the ASCII art, and just use a normal 404 page. We can keep it for the main site, but it feels weird to inject into users docs. We should do something a bit more default and with less personality, since we don't know the style of the docs we're inserting into.

@humitos humitos force-pushed the humitos/allow-custom-404-pages branch 2 times, most recently from 9d819f6 to f7ce417 Feb 4, 2019
We use the sphinx-notfound-page extension to create a generic page for
each project. By default we use the same Maze Found ASCII art, but the
user can customize it by defining specific configs from the sphinx-notfound-page.
@humitos humitos force-pushed the humitos/allow-custom-404-pages branch from f7ce417 to 9ac46ef Feb 4, 2019
@humitos
Copy link
Member Author

@humitos humitos commented Feb 6, 2019

I followed what we discussed and this PR is ready to review again and probably merge.

Copy link
Member

@ericholscher ericholscher left a comment

Looks good

<p>Try using the search box or go to the homepage.</p>
''',
}

Copy link
Member

@ericholscher ericholscher Feb 6, 2019

This is just for the future, right? We need to add the extension if we want it to do anything.

Copy link
Member Author

@humitos humitos Feb 6, 2019

This is just of our docs to start testing it.

The extension is installed in our requirements.

I can remove it if you consider that it's best to start testing this in another dumb project, though.

@ericholscher ericholscher merged commit 8f0c78d into master Feb 6, 2019
1 of 2 checks passed
@delete-merged-branch delete-merged-branch bot deleted the humitos/allow-custom-404-pages branch Feb 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants