[Bug 1385315] Use 302 not 301 for locale based redirect #4345
Conversation
|
Please fix the |
Codecov Report
@@ Coverage Diff @@
## master #4345 +/- ##
=======================================
Coverage 88.33% 88.33%
=======================================
Files 163 163
Lines 10233 10233
Branches 1420 1420
=======================================
Hits 9039 9039
Misses 968 968
Partials 226 226
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks @safwanrahman, this is what I asked for in PR #4321.
Can you combine the import statement for django.http? You'll have to use parenthesis like in kuma/core/tests/test_decorators.py
kuma/core/middleware.py
Outdated
| @@ -4,6 +4,7 @@ | |||
| from django.conf import settings | |||
| from django.core import urlresolvers | |||
| from django.http import HttpResponseForbidden, HttpResponsePermanentRedirect | |||
| from django.http import HttpResponseRedirect | |||
There was a problem hiding this comment.
This could be combined with the above import statement
| return HttpResponsePermanentRedirect(urlparams(new_path, **query)) | ||
|
|
||
| # Never use HttpResponsePermanentRedirect here. | ||
| # Its a temporary redirect and should return with http 302, not 301 |
There was a problem hiding this comment.
I'm not sure in this case. I could argue that:
https://developer.mozilla.org/en-US/docs/Mozilla?lang=de
should be a permanent redirect to:
https://developer.mozilla.org/de/docs/Mozilla
As far as I can tell from the code and the analytics, the ?lang=foo feature is never used. This path and the one test that exercises it should probably be dropped. But that's a new PR.
So 👍 for now, for consistency.
There was a problem hiding this comment.
Update: it looks like ?lang=de is used in the footer language selector when JS is disabled. And, because JS is disabled, it doesn't show up in Google Analytics 😄
There was a problem hiding this comment.
https://developer.mozilla.org/en-US/docs/Mozilla?lang=deshould be a permanent redirect to:
https://developer.mozilla.org/de/docs/Mozilla
Like, If I go to bn-BD locale for some reason, I dont want to be there permanently.
|
@jwhitlock fixed the nit. r? |
|
OK for now. Someday I'll turn on 80-column lines in |
We should redirect the visitor with 302 redirect rather than 301
@jwhitlock r?