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

Figure out why redirects aren't working #937

Closed
alanmoo opened this issue Jan 9, 2018 · 10 comments
Closed

Figure out why redirects aren't working #937

alanmoo opened this issue Jan 9, 2018 · 10 comments
Assignees

Comments

@alanmoo
Copy link
Contributor

alanmoo commented Jan 9, 2018

Redirects added at https://foundation.mozilla.org/admin/redirects/redirect/ aren't working (they 404). Let's figure out why.

@alanmoo
Copy link
Contributor Author

alanmoo commented Jan 9, 2018

It appears that locally it's doing a 301. That's...not ideal, as the redirect I want to use for now will eventually be disabled to hit pages served by Mezzanine

From https://docs.djangoproject.com/en/2.0/ref/contrib/redirects/
You can subclass RedirectFallbackMiddleware and set response_redirect_class to django.http.HttpResponseRedirect to use a 302 Moved Temporarily redirect instead.

@cadecairos
Copy link

Here's what I found out:

  • Only breaks when DEBUG=False
  • The view handler handler404 in urls.py is a RedirectView, so it's returning a 302 response to /errors/404 if the router can't match the requested path to a view.
  • When DEBUG=True handler404 doesn't execute, it seems.
  • Handler404 turns what would be 404s into 302s, so when the RedirectMiddlware is executed for the request, it doesn't do anything, because it only looks for 404s

In order to fix this, our handler404 can't set the response code to 302 before the redirect middleware executes. I see two ways forward:

  1. Make the 404 page a view in Django
  2. Remove the handler404 setting, and sub-class the RedirectMiddleware to do what we want (lookup redirects in the DB, or redirect to /error/404

@cadecairos
Copy link

@alanmoo This is still assigned to me - we need to make a decision as to which way to fix this. I'm okay with either of the options I laid out, however #2 diverges our code from Django/Mezzanine core even more, which might burn us in the future.

@alanmoo
Copy link
Contributor Author

alanmoo commented Jan 24, 2018

Then yeah, let's stick with option 1. What's the actual work required for that? It might be something we can leave up for a contributor to pick up if they're interested until someone on the team has a chance to get to it.

@alanmoo
Copy link
Contributor Author

alanmoo commented Mar 23, 2018

Will this be resolved once Wagtail is handling our 404's?

@cadecairos
Copy link

It should be resolved once we stop relying on a statically built frontend that's served via whitenoise.

@kristinashu
Copy link

Will redirects work now?

@cadecairos
Copy link

Not yet, we still need fix the problems outlined in this comment

@alanmoo
Copy link
Contributor Author

alanmoo commented Apr 17, 2018

We have stopped serving those pages statically, and I believe I added a djano-based 404 page last week.

@cadecairos
Copy link

if it's ready to go, we should update this line so that it's not a redirect to a static page.

@alanmoo alanmoo closed this as completed Apr 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants