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

Remove Django's LocaleMiddleware from the app #5701

Merged
merged 2 commits into from
May 5, 2020

Conversation

Scotchester
Copy link
Contributor

@Scotchester Scotchester commented May 4, 2020

Because we always activate Spanish-language content via the inclusion of
an /es/ path segment that triggers the setting of the language
context variable to es, we do not need this middleware for anything
currently, and its presence can cause unwanted appearance of Spanish on
English pages if a user has set their browser's primary language to
Spanish.

Removals

  • django.middleware.locale.LocaleMiddleware removed from MIDDLEWARE in our base settings file

Testing

  1. Pull branch
  2. Visit various Spanish-language pages on your local server and compare to how they look in production
  3. Update your browser settings to prefer Spanish (e.g. in Chrome, Settings, Advanced, Languages, Language)
  4. Visit /data-research/consumer-complaints/ using your favorite way to bypass Akamai, observe some parts of the global header and footer appearing in Spanish
  5. Visit http://localhost:8000/data-research/consumer-complaints/ and observe those same parts now in English
  6. Remember to set your browser language preferences back when done testing

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows the standards laid out in the CFPB development guidelines
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Visually tested in supported browsers and devices (see checklist below 👇)
  • Project documentation has been updated
  • Reviewers requested with the Reviewers tool ➡️

Testing checklist

Browsers

  • Chrome on desktop
  • Firefox
  • Safari on macOS
  • Edge
  • Internet Explorer 9, 10, and 11
  • Safari on iOS
  • Chrome on Android

Because we always activate Spanish-language content via the inclusion of 
an `/es/` path segment that triggers the setting of the `language` 
context variable to `es`, we do not need this middleware for anything 
currently, and its presence can cause unwanted appearance of Spanish on 
English pages if a user has set their browser's primary language to 
Spanish.
@Scotchester Scotchester requested a review from a team May 4, 2020 21:20
@higs4281
Copy link
Member

higs4281 commented May 4, 2020

Because we always activate Spanish-language content via the inclusion of
an /es/ path segment

This is not true. We have 63 Spanish blog pages that do not use an /es/ path segment, such as

/about-us/blog/errores-comunes-que-la-gente-encuentra-en-su-reporte-de-credito-y-como-corregirlos/

What drives translations for those and other Wagtail pages is the page.language setting.
The language setting doesn't depend on django.middleware.locale.LocaleMiddleware, so we can still drop it without upsetting our Spanish universe.

But I would lean toward honoring the preferences of those probably rare users who go to the trouble of setting their browsers' default language. If they want to see every translatable thing available, why not let them?

We do have an odd (and also rare) caching problem that this might solve, but for me that doesn't tip the scale.

@Scotchester
Copy link
Contributor Author

Because we always activate Spanish-language content via the inclusion of
an /es/ path segment

This is not true. We have 63 Spanish blog pages that do not use an /es/ path segment, such as

/about-us/blog/errores-comunes-que-la-gente-encuentra-en-su-reporte-de-credito-y-como-corregirlos/

What drives translations for those and other Wagtail pages is the page.language setting.
The language setting doesn't depend on django.middleware.locale.LocaleMiddleware, so we can still drop it without upsetting our Spanish universe.

Thanks for pointing that out, Bill; I didn't remember that.

But I would lean toward honoring the preferences of those probably rare users who go to the trouble of setting their browsers' default language. If they want to see every translatable thing available, why not let them?

Because the way that our caching system currently works, their browser language preference is already ignored, and Akamai returns the English page anyway. (Assuming that an English-language user was the first to visit the page after the cache was invalidated.)

We may want to investigate whether or not Akamai can return alternate versions of the page based on browser language settings.

@higs4281
Copy link
Member

higs4281 commented May 4, 2020

their browser language preference is already ignored ...

Not entirely, at least not in my testing. I just hit https://www.consumerfinance.gov/data-research/consumer-complaints/ in Chrome with my default language set to Spanish, and I got Spanish header and footer elements, including the search bar ("Buscar"), which could help a Spanish-only-speaker find something.

Copy link
Member

@chosak chosak left a comment

Choose a reason for hiding this comment

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

Because we always activate Spanish-language content via the inclusion of
an /es/ path segment

This is not true. We have 63 Spanish blog pages that do not use an /es/ path segment, such as

There are several other non-blog Spanish pages elsewhere in the tree, both Wagtail (ex: https://www.consumerfinance.gov/consumer-tools/sending-money/es/) and non-Wagtail (ex: https://www.consumerfinance.gov/consumer-tools/retirement/before-you-claim/es/).

@Scotchester
Copy link
Contributor Author

There are several other non-blog Spanish pages elsewhere in the tree, both Wagtail (ex: https://www.consumerfinance.gov/consumer-tools/sending-money/es/) and non-Wagtail (ex: https://www.consumerfinance.gov/consumer-tools/retirement/before-you-claim/es/).

These both have /es/ path segments, and correctly show the Spanish translations of global header and footer stuff, but the Spanish version of Before You Claim is not showing the Spanish mega menu.

@Scotchester
Copy link
Contributor Author

their browser language preference is already ignored ...

Not entirely, at least not in my testing. I just hit https://www.consumerfinance.gov/data-research/consumer-complaints/ in Chrome with my default language set to Spanish, and I got Spanish header and footer elements, including the search bar ("Buscar"), which could help a Spanish-only-speaker find something.

Well, that's surprising and confusing.

Copy link
Member

@willbarton willbarton left a comment

Choose a reason for hiding this comment

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

The thing this seems to give us is consistency between our Django and Jinja templates. The question of whether this is the correct approach to translation is... still very much open, but seems way above our paygrades.

Copy link
Member

@higs4281 higs4281 left a comment

Choose a reason for hiding this comment

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

I bow to simplicity.

@Scotchester Scotchester merged commit 287d58e into master May 5, 2020
@Scotchester Scotchester deleted the rm-localemiddleware branch May 5, 2020 14:29
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.

4 participants