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

Limit non-English locales #3661

Merged
merged 9 commits into from
May 3, 2021
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 0 additions & 22 deletions client/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,37 +25,15 @@ export const AUTOCOMPLETE_SEARCH_WIDGET = JSON.parse(
// We could encode the list in the SSR rendering but that means the client side
// code needs to depend on having access to the `window` global first.
export const VALID_LOCALES = new Set([
"ar",
"bg",
"bn",
"ca",
"de",
"el",
"en-US",
"es",
"fa",
"fi",
"fr",
"he",
"hi-IN",
"hu",
"id",
"it",
"ja",
"kab",
"ko",
"ms",
"my",
"nl",
"pl",
"pt-BR",
"pt-PT",
"ru",
"sv-SE",
"th",
"tr",
"uk",
"vi",
"zh-CN",
"zh-TW",
]);
Expand Down
13 changes: 8 additions & 5 deletions deployer/aws-lambda/content-origin-request/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,14 @@ exports.handler = async (event) => {
return redirect(`/${request.uri.replace(/^\/+/g, "")}`);
}

const { url, status } = resolveFundamental(request.uri);
let { url, status } = resolveFundamental(request.uri);
if (url) {
// TODO: Do we want to add the query string to the redirect?
// If we decide we do, then we probably need to change
// the caching policy on the "*/docs/* behavior" to
// cache based on the query strings as well.
// NOTE: The query string is not forwarded for document requests,
// as directed by their origin request policy, so it's safe to
// assume "request.querystring" is empty for document requests.
if (request.querystring) {
url += (url.includes("?") ? "&" : "?") + request.querystring;
}
return redirect(url, {
status,
cacheControlSeconds: THIRTY_DAYS,
Expand All @@ -127,6 +129,7 @@ exports.handler = async (event) => {
const path = request.uri.endsWith("/")
? request.uri.slice(0, -1)
: request.uri;
// Note that "getLocale" only returns valid locales, never a retired locale.
const locale = getLocale(request);
// The only time we actually want a trailing slash is when the URL is just
// the locale. E.g. `/en-US/` (not `/en-US`)
Expand Down
31 changes: 31 additions & 0 deletions deployer/aws-lambda/content-origin-request/server.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -207,3 +207,34 @@ describe("redirect double-slash prefix URIs", () => {
expect(r.headers["location"]).toBe("/blablabla");
});
});

describe("retired locale redirects", () => {
it("should 302 redirect a retired locale (accept-language)", async () => {
const r = await get("/", {
"Accept-language": "sv-SE",
});
expect(r.statusCode).toBe(302);
expect(r.headers["location"]).toBe("/en-US/");
});
it("should 302 redirect a retired locale (preferredlocale cookie)", async () => {
const r = await get("/docs/Web/HTTP", {
Cookie: "preferredlocale=it",
});
expect(r.statusCode).toBe(302);
expect(r.headers["location"]).toBe("/en-US/docs/Web/HTTP");
});
it("should 302 redirect a retired locale (no query string)", async () => {
const r = await get("/sv-SE/docs/Web/HTML");
expect(r.statusCode).toBe(302);
expect(r.headers["location"]).toBe(
"/en-US/docs/Web/HTML?retiredLocale=sv-SE"
);
});
it("should 302 redirect a retired locale (query string, improper locale)", async () => {
const r = await get("/BN/search?q=video");
expect(r.statusCode).toBe(302);
expect(r.headers["location"]).toBe(
"/en-US/search?retiredLocale=bn&q=video"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

);
});
});
30 changes: 18 additions & 12 deletions libs/constants/index.js
Original file line number Diff line number Diff line change
@@ -1,38 +1,43 @@
const VALID_LOCALES = new Map(
[
"de",
"en-US",
"es",
"fr",
"ja",
"ko",
"pl",
"pt-BR",
"ru",
"zh-CN",
"zh-TW",
].map((x) => [x.toLowerCase(), x])
);

const RETIRED_LOCALES = new Map(
[
"ar",
"bg",
"bn",
"ca",
"de",
"el",
"en-US",
"es",
"fa",
"fi",
"fr",
"he",
"hi-IN",
"hu",
"id",
"it",
"ja",
"kab",
"ko",
"ms",
"my",
"nl",
"pl",
"pt-BR",
"pt-PT",
"ru",
"sv-SE",
"th",
"tr",
"uk",
"vi",
"zh-CN",
"zh-TW",
].map((x) => [x.toLowerCase(), x])
);

Expand All @@ -41,7 +46,7 @@ const DEFAULT_LOCALE = "en-US";
const LOCALE_ALIASES = new Map([
// Case is not important on either the keys or the values.
["en", "en-us"],
["pt", "pt-PT"], // Note! Portugal Portugese is the default
["pt", "pt-br"],
["cn", "zh-cn"],
["zh", "zh-cn"],
["zh-hans", "zh-cn"],
Expand All @@ -64,6 +69,7 @@ const ACTIVE_LOCALES = new Set([
module.exports = {
ACTIVE_LOCALES,
VALID_LOCALES,
RETIRED_LOCALES,
DEFAULT_LOCALE,
LOCALE_ALIASES,
PREFERRED_LOCALE_COOKIE_NAME,
Expand Down
25 changes: 22 additions & 3 deletions libs/fundamental-redirects/index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
const { VALID_LOCALES, LOCALE_ALIASES } = require("../constants");
const {
VALID_LOCALES,
LOCALE_ALIASES,
RETIRED_LOCALES,
} = require("../constants");

const startRe = /^\^?\/?/;
const startTemplate = /^\//;
Expand Down Expand Up @@ -65,12 +69,12 @@ for (const locale of VALID_LOCALES.keys()) {
}

for (const [alias, correct] of LOCALE_ALIASES) {
// E.g. things like `en` -> `en-us` or `pt` -> `pt-pt`
// E.g. things like `en` -> `en-us` or `pt` -> `pt-br`
fixableLocales.set(alias, correct);
}

// All things like `/en_Us/docs/...` -> `/en-US/docs/...`
const LOCALE_PATTERNS = [
// All things like `/en_Us/docs/...` -> `/en-US/docs/...`
redirect(
new RegExp(
`^(?<locale>${Array.from(fixableLocales.keys()).join(
Expand All @@ -92,6 +96,21 @@ const LOCALE_PATTERNS = [
},
{ permanent: true }
),
// Retired locales
redirect(
new RegExp(
`^(?<locale>${Array.from(RETIRED_LOCALES.keys()).join(
"|"
)})(/(?<suffix>.*)|$)`,
"i"
),
({ locale, suffix }) => {
const join = suffix && suffix.includes("?") ? "&" : "?";
return `/en-US/${
escattone marked this conversation as resolved.
Show resolved Hide resolved
(suffix || "") + join
}retiredLocale=${RETIRED_LOCALES.get(locale.toLowerCase())}`;
}
),
];

// Redirects/rewrites/aliases migrated from SCL3 httpd config
Expand Down
4 changes: 2 additions & 2 deletions testing/integration/headless/map_301.py
Original file line number Diff line number Diff line change
Expand Up @@ -1069,8 +1069,8 @@
url_test("/EN-US/?next=FOO", "/en-US/?next=FOO", status_code=302),
url_test("/eN-us/docs/Web", "/en-US/docs/Web", status_code=302),
url_test("/eN-us/docs/Web/", "/en-US/docs/Web", status_code=302),
url_test("/eN-us/docs/Web?next=FOO", "/en-US/docs/Web?next=FOO", status_code=302),
url_test("/eN-us/docs/Web/?next=FOO", "/en-US/docs/Web?next=FOO", status_code=302),
url_test("/eN-us/docs/Web?next=FOO", "/en-US/docs/Web", status_code=302),
url_test("/eN-us/docs/Web/?next=FOO", "/en-US/docs/Web", status_code=302),
url_test("/en-uS/search", "/en-US/search", status_code=302),
url_test("/en-uS/search/", "/en-US/search", status_code=302),
url_test("/en-Us/search?q=video", "/en-US/search?q=video", status_code=302),
Expand Down
57 changes: 57 additions & 0 deletions testing/integration/headless/test_redirects.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import pytest

from . import request
from utils.urls import assert_valid_url

from .map_301 import (
Expand Down Expand Up @@ -116,3 +117,59 @@ def test_firefox_source_docs_redirects(url, base_url):
def test_misc_redirects(url, base_url):
url["base_url"] = base_url
assert_valid_url(**url)


@pytest.mark.parametrize(
"retired_locale",
(
"ar",
"bg",
"bn",
"ca",
"el",
"fa",
"fi",
"he",
"hi-IN",
"hu",
"id",
"it",
"kab",
"ms",
"my",
"nl",
"pt-PT",
"sv-SE",
"th",
"tr",
"uk",
"vi",
),
)
@pytest.mark.parametrize(
"slug",
[
"",
"/",
"/docs/Web",
"/docs/Web/",
"/search",
"/search/",
"/search?q=video",
"/search/?q=video",
"/signup",
"/settings",
],
)
def test_retired_locale_redirects(base_url, slug, retired_locale):
"""Ensure that requests for retired locales properly redirect."""
resp = request("get", f"{base_url}/{retired_locale}{slug}")
assert resp.status_code == 302
slug_parts = slug.split("?")
expected_slug = slug_parts[0].lstrip("/")
expected_qs = f"?retiredLocale={retired_locale}"
if len(slug_parts) > 1:
expected_qs += f"&{slug_parts[1]}"
assert (
resp.headers["Location"] == f"/en-US/{expected_slug}{expected_qs}"
), f"{resp.headers['Location']} is not /en-US/{expected_slug}{expected_qs}"
4 changes: 2 additions & 2 deletions testing/tests/developing.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -166,12 +166,12 @@ describe("Testing the Express server", () => {
const response = await got(serverURL("/"), {
followRedirect: false,
headers: {
Cookie: "preferredlocale=SV-se",
Cookie: "preferredlocale=ja",
"Accept-language": "fr-CH, fr;q=0.9, en;q=0.8, de;q=0.7, *;q=0.5",
},
});
expect(response.statusCode).toBe(302);
expect(response.headers.location).toBe("/sv-SE/");
expect(response.headers.location).toBe("/ja/");
});
});

Expand Down
6 changes: 3 additions & 3 deletions testing/tests/headless.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -218,10 +218,10 @@ describe("Basic viewing of functional pages", () => {
});

it("should suggest the en-US equivalent on non-en-US pages not found", async () => {
await page.goto(testURL("/sv-SE/docs/Web/foo"));
await page.goto(testURL("/ja/docs/Web/foo"));
await expect(page).toMatch("Page not found");
await expect(page).toMatch("/sv-SE/docs/Web/foo could not be found");
// Simply by swapping the "sv-SE" for "en-US" it's able to find the index.json
await expect(page).toMatch("/ja/docs/Web/foo could not be found");
// Simply by swapping the "ja" for "en-US" it's able to find the index.json
// for that slug and present a link to it.
await expect(page).toMatch("Good news!");
await expect(page).toMatchElement("a", {
Expand Down
Loading