refactor(i18n): improve locale handling by artem-vavilov · Pull Request #72511 · code-dot-org/code-dot-org

artem-vavilov · 2026-05-05T11:14:51Z

Refactors request-locale handling by removing the legacy VarnishEnvironment middleware and shifting locale resolution/cookie+redirect behavior into a new Middleware::I18n, making I18n.locale the request-locale source of truth across Dashboard (and updating related Global Edition behavior and tests).

Changes:

Replace VarnishEnvironment with Middleware::I18n in the Dashboard Rack middleware stack and remove controller-level locale wrapping.
Update Global Edition routing/redirect behavior and related tests to operate with I18n.locale as the authoritative locale.
Remove legacy shared Varnish middleware/tests and adjust request/test helpers to set locale via I18n.locale.

Links

Related PR: Use I18n.locale as the only source of truth #71930

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2108f5e6d3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…empty languages table

Copilot

Pull request overview

This PR refactors request-locale handling by removing the legacy VarnishEnvironment middleware and shifting locale resolution/cookie+redirect behavior into a new Middleware::I18n, making I18n.locale the request-locale source of truth across Dashboard (and updating related Global Edition behavior and tests).

Changes:

Replace VarnishEnvironment with Middleware::I18n in the Dashboard Rack middleware stack and remove controller-level locale wrapping.
Update Global Edition routing/redirect behavior and related tests to operate with I18n.locale as the authoritative locale.
Remove legacy shared Varnish middleware/tests and adjust request/test helpers to set locale via I18n.locale.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
shared/test/test_varnish_environment.rb	Removes legacy middleware unit test coverage tied to `VarnishEnvironment`.
shared/middleware/varnish_environment.rb	Deletes legacy locale/cookie/redirect middleware implementation.
pegasus/test/test_router.rb	Updates Pegasus router tests to use `I18n.locale` for localized rendering.
pegasus/config.ru	Removes `VarnishEnvironment` from the Pegasus Rack stack.
lib/cdo/rack/request.rb	Makes `request.locale` reflect `I18n.locale` instead of `env['cdo.locale']`.
lib/cdo/languages.rb	Adjusts HoC locale lookup behavior when no DB row exists.
lib/cdo/i18n.rb	Removes `language_change_url` helper that depended on `VarnishEnvironment`.
lib/cdo/global_edition.rb	Makes `region_locales` nil-safe by defaulting to `[]`.
dashboard/test/test_helper.rb	Switches request-locale helpers from `cdo.locale` env to `I18n.locale`.
dashboard/test/integration/language_test.rb	Updates integration assertions to validate rendered `lang` output rather than `request.locale`.
dashboard/test/integration/global_edition_test.rb	Updates GE integration expectations for the new middleware-driven locale flow.
dashboard/test/controllers/sessions_controller_test.rb	Updates controller test to derive locale from `I18n.locale`.
dashboard/test/controllers/home_controller_test.rb	Updates (skipped) test to use `I18n.locale`.
dashboard/lib/middleware/i18n.rb	Introduces new middleware to resolve locale (param/cookie/accept-language), set cookies, and redirect.
dashboard/lib/middleware/global_edition.rb	Updates GE middleware to use `I18n.locale` and adjust query-param handling.
dashboard/config/application.rb	Replaces middleware ordering to insert `Middleware::I18n` instead of `VarnishEnvironment`.
dashboard/app/controllers/test_controller.rb	Updates internal test endpoint to use `I18n.locale` for fallback locale.
dashboard/app/controllers/omniauth_callbacks_controller.rb	Updates locale cookie logic to compare against `I18n.locale`.
dashboard/app/controllers/home_controller.rb	Updates locale redirect parameter key to `Middleware::I18n::LOCALE_PARAM_KEY`.
dashboard/app/controllers/application_controller.rb	Removes controller `around_action` locale wrapping now handled by middleware.

Comments suppressed due to low confidence (1)

pegasus/test/test_router.rb:105

This test mutates global I18n.locale without restoring it, which can leak state into subsequent tests. Use I18n.with_locale('es-ES') { ... } (or reset I18n.locale in an ensure) to avoid cross-test coupling.

  def test_localized_markdown_fallback
    I18n.locale = 'es-ES'
    path = '/test_md'
    resp = get(path)
    assert_equal 200, resp.status, path
    assert_match "Hello", resp.body
  end

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…actoring

Copilot

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (2)

pegasus/config.ru:34

Removing VarnishEnvironment from the Pegasus Rack stack leaves no middleware that derives the request locale from language_, set_locale, or HTTP_ACCEPT_LANGUAGE. Since Cdo::RequestExtension#locale now reads I18n.locale, Pegasus requests will effectively always use whatever global locale happens to be set, breaking per-request localization. Add an i18n-localizing middleware back into pegasus/config.ru (either reuse the new middleware in a shared location or add a Pegasus-specific equivalent) so locale is set at the start of each request and set_locale redirects/cookies still work.


unless CDO.chef_managed
  # Only Chef-managed environments run an HTTP-cache service alongside the Rack app.
  # For other environments (development / CI), run the HTTP cache from Rack middleware.
  require 'cdo/rack/allowlist'

pegasus/test/test_router.rb:105

These tests now mutate global I18n.locale without restoring it, which can leak state into later tests and cause order-dependent failures. Wrap the request in I18n.with_locale(...) or save/restore the original locale in an ensure/teardown block.

  def test_localized_markdown
    I18n.locale = 'fr-FR'
    path = '/test_md'
    resp = get(path)
    assert_equal 200, resp.status, path
    assert_match "Bonjour", resp.body
  end

  def test_localized_markdown_fallback
    I18n.locale = 'es-ES'
    path = '/test_md'
    resp = get(path)
    assert_equal 200, resp.status, path
    assert_match "Hello", resp.body
  end

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

…cdo_cookie

wilkie · 2026-05-08T21:30:40Z

-          resolved_locale = nil if Cdo::GlobalEdition.locales_regions[resolved_locale].present?
-        end
+        return original_locale if Cdo::GlobalEdition.locale_available?(region, original_locale)
+        return url_locale      if Cdo::GlobalEdition.locale_available?(region, url_locale)


beautiful refactor, here. reduces a lot of the confusion and reliance on direct access to properties in the global edition module.

wilkie · 2026-05-08T21:42:00Z

+        # Hints to the browser which cookies to retain first when domain storage limits are reached.
+        self.set_cookie_header = "#{set_cookie_header}; priority=#{priority}" if priority
+      else
+        delete_cookie(key, global_data)


This is an excellent helper.

wilkie

🔥 varnish environment 🔥 🎉 🎉

Hamms

I definitely don't fully understand all the connections between different things that are getting touched here, but I appreciate how much more consistent and contained this makes everything! And the parts I do understand make sense to me.

I have a few minor questions, but otherwise this LGTM. Thanks so much for taking on this cleanup!

Hamms · 2026-05-11T23:01:11Z

+      end
+
+      private def param_locale
+        return @param_locale if defined?(@param_locale)


Is there a specific reason why we want to cache these _locale methods more aggressively in this new middleware than we were doing before in the old one?

You are right, we do not need this

Hamms · 2026-05-11T23:04:05Z

@@ -16,7 +16,6 @@ class LanguageTest < ActionDispatch::IntegrationTest
        follow_redirect! while response.status == 302
        must_respond_with :success

-        _(request.locale).must_equal expected_locale


Is there an equivalent check we can replace the checks with here and below, rather than removing them entirely?

Yes, the check below it:

code-dot-org/dashboard/test/integration/language_test.rb

Line 19 in 0ce0a09

must_select "html[lang='#{expected_locale}']"

code-dot-org/dashboard/app/views/layouts/application.html.haml

Lines 4 to 7 in 0ce0a09

- html_attrs = {dir: locale_dir, lang: locale}

- html_attrs['data-brand'] = brand if brand != Cdo::Brand::BRAND_CODE_ORG

- html_attrs['data-ge-region'] = request.ge_region if request.ge_region.present?

%html{html_attrs}

Hamms · 2026-05-11T23:07:50Z

@@ -214,17 +214,12 @@ class GlobalEditionTest < ActionDispatch::IntegrationTest
        end

        it 'redirects to international page with extra params and selected locale' do
-          expect(Metrics::Events).to receive(:log_event).with(
+          expect(Metrics::Events).not_to receive(:log_event).with(


I'm not particularly familiar with the internals of global_edition, and I'm having a hard time following the logic of why this changed. Can you help me understand both why we're no longer logging changes here, and why that's desirable?

…actoring

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fa5507435f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

This reverts commit 90db92c.

This reverts commit 1bdc8ee.

chatgpt-codex-connector Bot reviewed May 5, 2026

View reviewed changes

Comment thread dashboard/lib/middleware/global_edition.rb

Comment thread dashboard/lib/middleware/i18n.rb Outdated

artem-vavilov requested review from a team and wilkie May 5, 2026 12:22

refactor(i18n): improve i18n handling

28269b1

artem-vavilov force-pushed the dashboard-i18n-refactoring branch from 2108f5e to 28269b1 Compare May 6, 2026 14:47

fix(pegasus): update Languages.get_hoc_unique_language_by_locale for …

be309b6

…empty languages table

artem-vavilov requested review from a team and Copilot May 7, 2026 14:32

Copilot started reviewing on behalf of artem-vavilov May 7, 2026 16:32 View session

Copilot AI reviewed May 7, 2026

View reviewed changes

Comment thread dashboard/lib/middleware/i18n.rb Outdated

Comment thread dashboard/lib/middleware/i18n.rb

Comment thread dashboard/lib/middleware/global_edition.rb

Comment thread lib/cdo/languages.rb

Comment thread pegasus/test/test_router.rb

artem-vavilov added 3 commits May 7, 2026 23:41

Merge remote-tracking branch 'origin/staging' into dashboard-i18n-ref…

9ab2950

…actoring

fix(global-edition): fix locale setting

ce7820b

fix(pegasus): prevent SQL injection

aa753ac

artem-vavilov requested a review from Copilot May 7, 2026 22:10

refactor(i18n): optimize request chain when setting locale from param

b57709c

artem-vavilov force-pushed the dashboard-i18n-refactoring branch from 36da35c to b57709c Compare May 7, 2026 22:12

Copilot started reviewing on behalf of artem-vavilov May 7, 2026 22:12 View session

Copilot AI reviewed May 7, 2026

View reviewed changes

Comment thread lib/cdo/rack/request.rb

Comment thread lib/cdo/languages.rb Outdated

Comment thread dashboard/lib/middleware/global_edition.rb

Comment thread dashboard/lib/middleware/i18n.rb

Potential fix for pull request finding

ca0c599

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

artem-vavilov force-pushed the dashboard-i18n-refactoring branch from e45181c to 2e098e0 Compare May 8, 2026 14:36

refactor(middleware): replace cookies helper with Rack::Response#set_…

0ce0a09

…cdo_cookie

artem-vavilov force-pushed the dashboard-i18n-refactoring branch from 2e098e0 to 0ce0a09 Compare May 8, 2026 14:41

wilkie reviewed May 8, 2026

View reviewed changes

wilkie approved these changes May 8, 2026

View reviewed changes

artem-vavilov changed the title ~~refactor(i18n): improve i18n handling~~ refactor(i18n): improve locale handling May 11, 2026

artem-vavilov requested a review from Hamms May 11, 2026 20:43

Hamms approved these changes May 11, 2026

View reviewed changes

artem-vavilov added 2 commits May 12, 2026 03:20

Merge remote-tracking branch 'origin/staging' into dashboard-i18n-ref…

c760c65

…actoring

refactor(i18n): cache methods less aggressively

988959e

artem-vavilov added 2 commits May 12, 2026 03:38

chore(i18n): cleanup

90db92c

fix(global-edition): resolve locale setting issue when region is nil

fa55074

artem-vavilov requested a review from Copilot May 12, 2026 01:50

Copilot started reviewing on behalf of artem-vavilov May 12, 2026 01:52 View session

chatgpt-codex-connector Bot reviewed May 12, 2026

View reviewed changes

Comment thread lib/cdo/rack/request.rb

Revert "chore(i18n): cleanup"

7b7b452

This reverts commit 90db92c.

This comment was marked as resolved.

Sign in to view

artem-vavilov merged commit 1bdc8ee into staging May 12, 2026
8 checks passed

artem-vavilov deleted the dashboard-i18n-refactoring branch May 12, 2026 02:22

Hamms mentioned this pull request May 12, 2026

Revert "refactor(i18n): improve locale handling" #72639

Merged

Hamms added a commit that referenced this pull request May 12, 2026

Revert "refactor(i18n): improve locale handling (#72511)" (#72639)

65ff349

This reverts commit 1bdc8ee.

artem-vavilov mentioned this pull request May 12, 2026

refactor(i18n): improve locale handling #72641

Merged

	- html_attrs = {dir: locale_dir, lang: locale}
	- html_attrs['data-brand'] = brand if brand != Cdo::Brand::BRAND_CODE_ORG
	- html_attrs['data-ge-region'] = request.ge_region if request.ge_region.present?
	%html{html_attrs}

Conversation

artem-vavilov commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes:

Links

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wilkie May 8, 2026

Choose a reason for hiding this comment

Uh oh!

wilkie May 8, 2026

Choose a reason for hiding this comment

Uh oh!

wilkie left a comment

Choose a reason for hiding this comment

Uh oh!

Hamms left a comment

Choose a reason for hiding this comment

Uh oh!

Hamms May 11, 2026

Choose a reason for hiding this comment

Uh oh!

artem-vavilov May 12, 2026

Choose a reason for hiding this comment

Uh oh!

Hamms May 11, 2026

Choose a reason for hiding this comment

Uh oh!

artem-vavilov May 12, 2026

Choose a reason for hiding this comment

Uh oh!

Hamms May 11, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

artem-vavilov commented May 5, 2026 •

edited

Loading