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

Limit non-English locales #3661

merged 9 commits into from
May 3, 2021

Conversation

escattone
Copy link
Contributor

@escattone escattone commented Apr 29, 2021

Fixes #3665

Part of #3642

TODO (after merge):

  • Update */docs/* Cloudfront behavior to use MDN-None forwarding policy on prod & dev (stage as already been done)
  • Update the version of the content-origin-request lambda@edge function in our dev, stage, and prod Cloudfront behaviors
  • Remove retired locales from stage and prod S3

@escattone escattone requested review from peterbe and Gregoor April 29, 2021 20:07
@escattone escattone changed the title Limit locales 3642 Limit non-English locales Apr 29, 2021
@escattone
Copy link
Contributor Author

@peterbe @Gregoor I don't understand why these tests are failing.

@escattone
Copy link
Contributor Author

@peterbe @Gregoor I just discovered one thing I need to do is remove any URL's with retired locales from the _redirects.txt files in content and translated-content.

@escattone
Copy link
Contributor Author

@peterbe @Gregoor I don't understand why these tests are failing.

@peterbe @Gregoor This was due to the presence of a URL with a retired locale in the content repo's files/en-us/_redirects.txt. All good now (see mdn/content#4627). This ready for review.

@escattone
Copy link
Contributor Author

escattone commented May 2, 2021

@pertebe @Gregoor FYI, at first I implemented the retired-locale redirects directly in the lambda function, but then thought that perhaps a better place for that functionality was in the fundamental redirects (e.g., links to retired locales would be detected as flaws). As it stands, this PR places the redirect code in the fundamental redirects, but if you simply remove the last commit, the placement moves back into the lambda function. So we can easily go either way, depending on what we all think is best.

@Gregoor
Copy link
Contributor

Gregoor commented May 3, 2021

Using the flaws sounds like a great idea to me and the codes does cohere to me. I am a little sad that we don't have code-sharing between the client and lib's constants, but it's only incidentally the first time I am noticing it and not too related to this PR (plus you kept it in sync here), so that's just a note on the side.
I think Peter is in a better position to give this the final greenlight, but it lgtm 👍🏼

Copy link
Contributor

@peterbe peterbe left a comment

Choose a reason for hiding this comment

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

As per discussed, we should make the unit tests for the lambda function also test this stuff at least once. E.g. /sv-SE/docs/Foo?utm=bla

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!

@peterbe peterbe merged commit 974f86a into mdn:main May 3, 2021
@escattone escattone deleted the limit-locales-3642 branch May 3, 2021 21:04
peterbe pushed a commit to peterbe/yari that referenced this pull request Jun 1, 2021
* update/add locale constants

* add and test redirects of retired locales

* fix locale alias for pt

* fix for developing.test.js cookie test

* fix gather-git-history

* add integration tests for retired locale redirects

* move redirects into the fundamental redirects

* add lambda tests (feedbacked)

* use DEFAULT_LOCALE (feedbacked)
wbamberg pushed a commit to wbamberg/yari that referenced this pull request Jul 29, 2021
* upstream/main: (288 commits)
  remove tests for obsolete search endpoints (mdn#3688)
  Limit non-English locales (mdn#3661)
  move footer icons to CSS bg images (mdn#3655)
  not all http:// links can be https:// (mdn#3657)
  add the lost part and update the translation for Express (mdn#3664)
  update the translation of the zh-TW (mdn#3663)
  Add a Specifications macro and extract a special specification section to the Document (mdn#3518)
  build(deps-dev): bump & migrate use-debounce to 6.0.0 (mdn#3684)
  build(deps-dev): bump puppeteer from 9.0.0 to 9.1.0 (mdn#3686)
  build(deps-dev): bump & migrate husky to 6.0.0 (mdn#3681)
  build(deps-dev): bump jest-environment-jsdom-sixteen from 1.0.3 to 2.0.0 (mdn#3615)
  More concurrent open npm PRs (mdn#3685)
  build(deps-dev): bump black from 21.4b0 to 21.4b2 in /deployer (mdn#3668)
  build(deps-dev): bump black in /testing/integration (mdn#3666)
  build(deps-dev): bump @babel/core from 7.13.16 to 7.14.0 (mdn#3683)
  build(deps-dev): bump jest-puppeteer from 5.0.2 to 5.0.3 (mdn#3680)
  build(deps): bump @mdn/browser-compat-data from 3.3.0 to 3.3.2 (mdn#3679)
  build(deps-dev): bump sass from 1.32.11 to 1.32.12 (mdn#3678)
  build(deps-dev): bump eslint-plugin-jest from 24.3.5 to 24.3.6 (mdn#3677)
  build(deps): bump http-proxy-middleware from 1.3.0 to 1.3.1 (mdn#3675)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server start error due to _githistory.json [retired languages]
3 participants