From 881a6d2212e1cfe0f9ac5fcbfe3f9992ab18e3b6 Mon Sep 17 00:00:00 2001 From: Peter Bengtsson Date: Fri, 4 Mar 2022 16:56:57 -0500 Subject: [PATCH] Revert "redirect to your preferred language (#25664)" (#25869) This reverts commit a9947c086cbc3d1cfd2a7f0e5ead6692b76d4e75. --- components/page-header/LanguagePicker.tsx | 29 +------- lib/get-redirect.js | 11 ++- middleware/detect-language.js | 31 +++----- middleware/redirects/handle-redirects.js | 31 ++++---- tests/routing/redirects.js | 86 ++--------------------- tests/unit/get-redirect.js | 14 ---- 6 files changed, 44 insertions(+), 158 deletions(-) diff --git a/components/page-header/LanguagePicker.tsx b/components/page-header/LanguagePicker.tsx index a98362257b..f4ac20ef6f 100644 --- a/components/page-header/LanguagePicker.tsx +++ b/components/page-header/LanguagePicker.tsx @@ -1,14 +1,9 @@ import { useRouter } from 'next/router' -import Cookies from 'js-cookie' - import { Link } from 'components/Link' import { useLanguages } from 'components/context/LanguagesContext' import { Picker } from 'components/ui/Picker' import { useTranslation } from 'components/hooks/useTranslation' -// This value is replicated in two places! See middleware/detect-language.js -const PREFERRED_LOCALE_COOKIE_NAME = 'preferredlang' - type Props = { variant?: 'inline' } @@ -27,22 +22,6 @@ export const LanguagePicker = ({ variant }: Props) => { // in a "denormalized" way. const routerPath = router.asPath.split('#')[0] - function rememberPreferredLanguage(code: string) { - try { - Cookies.set(PREFERRED_LOCALE_COOKIE_NAME, code, { - expires: 365, - secure: document.location.protocol !== 'http:', - }) - } catch (err) { - // You can never be too careful because setting a cookie - // can fail. For example, some browser - // extensions disallow all setting of cookies and attempts - // at the `document.cookie` setter could throw. Just swallow - // and move on. - console.warn('Unable to set preferred language cookie', err) - } - } - return ( { text: lang.nativeName || lang.name, selected: lang === selectedLang, item: ( - { - rememberPreferredLanguage(lang.code) - }} - > + {lang.nativeName ? ( <> {lang.nativeName} ( diff --git a/lib/get-redirect.js b/lib/get-redirect.js index 52810ac88d..d9c5b5ec81 100644 --- a/lib/get-redirect.js +++ b/lib/get-redirect.js @@ -9,9 +9,9 @@ const nonEnterpriseDefaultVersionPrefix = `/${nonEnterpriseDefaultVersion}` // Return the new URI if there is one, otherwise return undefined. export default function getRedirect(uri, context) { - const { redirects, userLanguage } = context + const { redirects, pages } = context - let language = userLanguage || 'en' + let language = 'en' let withoutLanguage = uri if (languagePrefixRegex.test(uri)) { language = uri.match(languagePrefixRegex)[1] @@ -109,7 +109,12 @@ export default function getRedirect(uri, context) { } if (basicCorrection) { - return getRedirect(basicCorrection, context) || basicCorrection + return ( + getRedirect(basicCorrection, { + redirects, + pages, + }) || basicCorrection + ) } if (withoutLanguage.startsWith('/admin/')) { diff --git a/middleware/detect-language.js b/middleware/detect-language.js index 9472e8d36c..1d222ede5b 100644 --- a/middleware/detect-language.js +++ b/middleware/detect-language.js @@ -1,17 +1,14 @@ -import languages, { languageKeys } from '../lib/languages.js' +import libLanguages from '../lib/languages.js' import parser from 'accept-language-parser' +const languageCodes = Object.keys(libLanguages) const chineseRegions = ['CN', 'HK'] -// This value is replicated in two places! See component. -// Note, the only reason this is exported is to benefit the tests. -export const PREFERRED_LOCALE_COOKIE_NAME = 'preferredlang' - function translationExists(language) { if (language.code === 'zh') { return chineseRegions.includes(language.region) } - return languageKeys.includes(language.code) + return languageCodes.includes(language.code) } function getLanguageCode(language) { @@ -20,41 +17,33 @@ function getLanguageCode(language) { function getUserLanguage(browserLanguages) { try { + let userLanguage = getLanguageCode(browserLanguages[0]) let numTopPreferences = 1 for (let lang = 0; lang < browserLanguages.length; lang++) { // If language has multiple regions, Chrome adds the non-region language to list if (lang > 0 && browserLanguages[lang].code !== browserLanguages[lang - 1].code) numTopPreferences++ if (translationExists(browserLanguages[lang]) && numTopPreferences < 3) { - return getLanguageCode(browserLanguages[lang]) + userLanguage = getLanguageCode(browserLanguages[lang]) + break } } + return userLanguage } catch { return undefined } } -function getUserLanguageFromCookie(req) { - const value = req.cookies[PREFERRED_LOCALE_COOKIE_NAME] - // But if it's a WIP language, reject it. - if (value && languages[value] && !languages[value].wip) { - return value - } -} - // determine language code from a path. Default to en if no valid match export function getLanguageCodeFromPath(path) { const maybeLanguage = (path.split('/')[path.startsWith('/_next/data/') ? 4 : 1] || '').slice(0, 2) - return languageKeys.includes(maybeLanguage) ? maybeLanguage : 'en' + return languageCodes.includes(maybeLanguage) ? maybeLanguage : 'en' } export default function detectLanguage(req, res, next) { req.language = getLanguageCodeFromPath(req.path) // Detecting browser language by user preference - req.userLanguage = getUserLanguageFromCookie(req) - if (!req.userLanguage) { - const browserLanguages = parser.parse(req.headers['accept-language']) - req.userLanguage = getUserLanguage(browserLanguages) - } + const browserLanguages = parser.parse(req.headers['accept-language']) + req.userLanguage = getUserLanguage(browserLanguages) return next() } diff --git a/middleware/redirects/handle-redirects.js b/middleware/redirects/handle-redirects.js index 22f81c9705..d799f161a5 100644 --- a/middleware/redirects/handle-redirects.js +++ b/middleware/redirects/handle-redirects.js @@ -1,6 +1,6 @@ import patterns from '../../lib/patterns.js' import { URL } from 'url' -import { pathLanguagePrefixed } from '../../lib/languages.js' +import languages, { pathLanguagePrefixed } from '../../lib/languages.js' import getRedirect from '../../lib/get-redirect.js' import { cacheControlFactory } from '../cache-control.js' @@ -13,7 +13,16 @@ export default function handleRedirects(req, res, next) { // blanket redirects for languageless homepage if (req.path === '/') { - const language = getLanguage(req) + let language = 'en' + + // if set, redirect to user's preferred language translation or else English + if ( + req.context.userLanguage && + languages[req.context.userLanguage] && + !languages[req.context.userLanguage].wip + ) { + language = req.context.userLanguage + } // Undo the cookie setting that CSRF sets. res.removeHeader('set-cookie') @@ -61,12 +70,17 @@ export default function handleRedirects(req, res, next) { // needs to become `/en/authentication/connecting-to-github-with-ssh` const possibleRedirectTo = `/en${req.path}` if (possibleRedirectTo in req.context.pages) { - const language = getLanguage(req) - + // As of Jan 2022 we always redirect to `/en` if the URL doesn't + // specify a language. ...except for the root home page (`/`). + // It's unfortunate but that's how it currently works. + // It's tracked in #1145 + // Perhaps a more ideal solution would be to do something similar to + // the code above for `req.path === '/'` where we look at the user + // agent for a header and/or cookie. // Note, it's important to use `req.url` here and not `req.path` // because the full URL can contain query strings. // E.g. `/foo?json=breadcrumbs` - redirect = `/${language}${req.url}` + redirect = `/en${req.url}` } } @@ -98,13 +112,6 @@ export default function handleRedirects(req, res, next) { return res.redirect(permanent ? 301 : 302, redirect) } -function getLanguage(req, default_ = 'en') { - // req.context.userLanguage, if it truthy, is always a valid supported - // language. It's whatever was in the user's request but filtered - // based on non-WIP languages in lib/languages.js - return req.context.userLanguage || default_ -} - function usePermanentRedirect(req) { // If the redirect was to essentially swap `enterprise-server@latest` // for `enterprise-server@3.x` then, we definitely don't want to diff --git a/tests/routing/redirects.js b/tests/routing/redirects.js index fdc9340f04..27dd42623b 100644 --- a/tests/routing/redirects.js +++ b/tests/routing/redirects.js @@ -2,14 +2,12 @@ import { fileURLToPath } from 'url' import path from 'path' import { isPlainObject } from 'lodash-es' import supertest from 'supertest' -import { jest } from '@jest/globals' - import createApp from '../../lib/app.js' import enterpriseServerReleases from '../../lib/enterprise-server-releases.js' import Page from '../../lib/page.js' import { get } from '../helpers/supertest.js' import versionSatisfiesRange from '../../lib/version-satisfies-range.js' -import { PREFERRED_LOCALE_COOKIE_NAME } from '../../middleware/detect-language.js' +import { jest } from '@jest/globals' const __dirname = path.dirname(fileURLToPath(import.meta.url)) @@ -134,28 +132,6 @@ describe('redirects', () => { expect(res.headers.location).toBe('/ja') expect(res.headers['cache-control']).toBe('private, no-store') }) - test('homepage redirects to preferred language by cookie', async () => { - const res = await get('/', { - headers: { - Cookie: `${PREFERRED_LOCALE_COOKIE_NAME}=ja`, - 'Accept-Language': 'es', // note how this is going to be ignored - }, - }) - expect(res.statusCode).toBe(302) - expect(res.headers.location).toBe('/ja') - expect(res.headers['cache-control']).toBe('private, no-store') - }) - test('homepage redirects to preferred language by cookie if valid', async () => { - const res = await get('/', { - headers: { - Cookie: `${PREFERRED_LOCALE_COOKIE_NAME}=xy`, - 'Accept-Language': 'ja', // note how this is going to be ignored - }, - }) - expect(res.statusCode).toBe(302) - expect(res.headers.location).toBe('/ja') - expect(res.headers['cache-control']).toBe('private, no-store') - }) }) describe('external redirects', () => { @@ -173,63 +149,13 @@ describe('redirects', () => { }) describe('localized redirects', () => { - const redirectFrom = - '/desktop/contributing-to-projects/changing-a-remote-s-url-from-github-desktop' - const redirectTo = - '/desktop/contributing-and-collaborating-using-github-desktop/working-with-your-remote-repository-on-github-or-github-enterprise/changing-a-remotes-url-from-github-desktop' - test('redirect_from for renamed pages', async () => { - const { res } = await get(`/ja${redirectFrom}`) + const { res } = await get( + '/ja/desktop/contributing-to-projects/changing-a-remote-s-url-from-github-desktop' + ) expect(res.statusCode).toBe(301) - const expected = `/ja${redirectTo}` - expect(res.headers.location).toBe(expected) - }) - - test('redirect_from for renamed pages by Accept-Language header', async () => { - const { res } = await get(redirectFrom, { - headers: { - 'Accept-Language': 'ja', - }, - }) - expect(res.statusCode).toBe(302) - const expected = `/ja${redirectTo}` - expect(res.headers.location).toBe(expected) - }) - - test('redirect_from for renamed pages but ignore Accept-Language header if not recognized', async () => { - const { res } = await get(redirectFrom, { - headers: { - // None of these are recognized - 'Accept-Language': 'sv,fr,gr', - }, - }) - expect(res.statusCode).toBe(302) - const expected = `/en${redirectTo}` - expect(res.headers.location).toBe(expected) - }) - - test('redirect_from for renamed pages but ignore unrecognized Accept-Language header values', async () => { - const { res } = await get(redirectFrom, { - headers: { - // Only the last one is recognized - 'Accept-Language': 'sv,ja', - }, - }) - expect(res.statusCode).toBe(302) - const expected = `/ja${redirectTo}` - expect(res.headers.location).toBe(expected) - }) - - test('will inject the preferred language from cookie', async () => { - const { res } = await get(redirectFrom, { - headers: { - Cookie: `${PREFERRED_LOCALE_COOKIE_NAME}=ja`, - 'Accept-Language': 'es', // note how this is going to be ignored - }, - }) - // 302 because the redirect depended on cookie - expect(res.statusCode).toBe(302) - const expected = `/ja${redirectTo}` + const expected = + '/ja/desktop/contributing-and-collaborating-using-github-desktop/working-with-your-remote-repository-on-github-or-github-enterprise/changing-a-remotes-url-from-github-desktop' expect(res.headers.location).toBe(expected) }) }) diff --git a/tests/unit/get-redirect.js b/tests/unit/get-redirect.js index ad425e0be3..5ad969f6ed 100644 --- a/tests/unit/get-redirect.js +++ b/tests/unit/get-redirect.js @@ -148,18 +148,4 @@ describe('getRedirect basics', () => { // it already has the enterprise-server prefix. expect(getRedirect('/enterprise-server/foo', ctx)).toBe(`/en/enterprise-server@${latest}/bar`) }) - - it('should redirect according to the req.context.userLanguage', () => { - const ctx = { - pages: {}, - redirects: { - '/foo': '/bar', - }, - userLanguage: 'ja', - } - expect(getRedirect('/foo', ctx)).toBe(`/ja/bar`) - // falls back to 'en' if it's falsy - ctx.userLanguage = null - expect(getRedirect('/foo', ctx)).toBe(`/en/bar`) - }) })