Proxito: look for custom 404 pages in the parent project of subprojects

[humitos]

After implementing #353 we do support custom 404 pages if we find a 404.html page in the current version (or default version) of the project.

Although, if we are accessing a subproject this logic does not work and the standard 404 page is served.

My guess is that the problem is at

https://github.com/rtfd/readthedocs.org/blob/2e29b61a815994b646dda204b975e1c8d7926b79/readthedocs/core/views/__init__.py#L193

Since that function checks for request.slug and if it does exist it just return it, but that request.slug is always set to the superproject at

https://github.com/rtfd/readthedocs.org/blob/2e29b61a815994b646dda204b975e1c8d7926b79/readthedocs/core/middleware.py#L71

or here if it's a custom domain,

https://github.com/rtfd/readthedocs.org/blob/2e29b61a815994b646dda204b975e1c8d7926b79/readthedocs/core/middleware.py#L85

In the end, the subproject is never considered when looking for a custom 404.

I'm not sure how big will be the impact of setting the subproject's slug to request.slug (which seems more accurate to me) in other places of the codebase.

[rshrc]

Is someone working on this issue currently?

[humitos]

@rshrc I don't think so. It's labeled as design decision.

Please, feel free to analyze its context and give and propose a detailed solution. It would be good to make the discussion happen and work towards the solution.

[humitos]

Also, I think there is another discussion that can take place here. There are two possible paths to take here,

1. setup a 404.html page on the superproject and that page should be server for all the subprojects (current behavior, but broken)
2. each subproject should have their own custom 404 at their root path (behavior proposed on the description by changing the request.slug)

[ericholscher]

We shouldn't change request.slug I don't think, but we definitely should be setting request.subproject or similar, and checking for that. I thought we were already doing something like that.

This is another place where centralizing our logic around this stuff would be really useful. We do have the map_subproject_slug decorator, but we should probably standardize that logic in a nicer way.

[ericholscher]

For the design decision:

• I think we should check for a subproject 404 page, then the parent project 404, and then our 404. A similar fallback approach to what we're doing now that lets users have more control over subprojects if they want it .

[humitos]

@ericholscher that makes sense to me.

To follow that direction, I propose:

1. update the middleware to set request.subproject_slug
2. update the project_and_path_from_request function to return the subproject slug also
3. make server_error_404_subdomain to give priority to the subproject, then project and finally fallback to our 404 page

I'm removing design decision and adding accepted so we can start moving in this direction.

This is another place where centralizing our logic around this stuff would be really useful. We do have the map_subproject_slug decorator, but we should probably standardize that logic in a nicer way.

I agree with this, but it's something that we can handle in another issue.

[stsewd]

I don't think this is a problem anymore, currently we are using django's URL resolve + _get_project_data_from_request to resolve the current project, which will be set to the subproject correctly.

readthedocs.org/readthedocs/proxito/views/serve.py

Lines 465 to 472 in cd8f2da

	_, __, kwargs = url_resolve(
	proxito_path,
	urlconf='readthedocs.proxito.urls',
	)
	version_slug = kwargs.get('version_slug')
	version_slug = self.get_version_from_host(request, version_slug)
	final_project, lang_slug, version_slug, filename = _get_project_data_from_request( # noqa

(new proxito implementation doesn't have this problem either)

And quick test to confirm

https://test.org.stsewd.dev/projects/stsewd-demo/en/fasfd

You can see the source code to check that the 404 page served was from the subproject.

What we don't do yet is this

I think we should check for a subproject 404 page, then the parent project 404

[ericholscher]

What we don't do yet is this

I think we should check for a subproject 404 page, then the parent project 404

Should we do this?

[stsewd]

What we don't do yet is this

I think we should check for a subproject 404 page, then the parent project 404

Should we do this?

I'm -1, since that would require more calls to storage :)

[ericholscher]

Makes sense to me :)