fix no-JS lang switcher (#5931)
One-line summary
Fixes the no javascript language switcher that sits in the footer.
Significant changes and points to review
- added check in l10n_util render function for any a query parameter called lang.
Issue / Bugzilla link
#5931
Testing
disable JS on your browser, try using the language switcher.
Not sure what needs to be looked at to ensure this fully works
Tested on Firefox and Chrome.
- [x] Tried switching languages on root path /
- [x] Tried switching languages on a non root path
- [x] Tried switching from english to cn and the redirect worked.
- [x] Tried going to non locale page (careers) and manually putting in parameters into the url -- something like localhost:8000/en-US/careers/?lang=da#. page did not break and parameters were removed on refresh.
Closing stale PR, please feel free to reopen should you have time to return to this. Thanks!
I think at this point I was just waiting for a review
edit: ready for review!
Hi Alex, doesn't look like I have the permissions to reopen this PR. I think it's ready for review, if you could reopen
@Ingi-Hong re-opened. If you think this PR is ready for review, can you please update it from draft status? Thanks
@stevejalim no rush on this one, but can you please take a look here? I'm not sure if there are any downside or potential problems with this solution. The other alternative would be to only show the form if JS is enabled (which I think would be fine also fwiw).
@Ingi-Hong whilst you're waiting on review here, I noticed there are some linting errors that need fixing: https://github.com/mozilla/bedrock/actions/runs/6773261630/job/18407712116?pr=13336
@Ingi-Hong - hi, are you still working on this?
Hi Alex, sorry I turned off notifications for this account to test something and forgot to turn it back on. I deleted the line that clears the query per your comment.
Hi @Ingi-Hong - looks like there are some small formatting issues to fix up:
lib/l10n_utils/__init__.py:119:71: W291 [*] Trailing whitespace
lib/l10n_utils/__init__.py:120:24: Q000 [*] Single quotes found but double quotes preferred
lib/l10n_utils/__init__.py:121:34: Q000 [*] Single quotes found but double quotes preferred
Found 3 errors.
[*] 3 fixable with the `--fix` option.
If you hve pre-commit installed you can install all the hooks we use with pre-commit install-hooks then run them all with pre-commit run -a -- it should catch the things noted above. (Or you can install ruff directly and do ruff . to format just the Python files.) Both approaches will fix the formatting and you can stage, commit and push up the changes. Thanks!
@stevejalim I'll be honest I don't feel like I have a good understanding of the possible repercussions or side effects of this change, give the area of code where this change lives.
Perhaps another back-ender might be able to spot something I'd likely miss?
No worries @alexgibson!
@robhudson I recall you're familiar with our locale-related logic. Could you give this an r, too, with a particular eye open for gotchas of the change, please?
This seems okay at first glance. Basically anywhere we use our own lib.l10n_utils.render we would be able to override the chosen language for that page with a ?lang={lang} query string parameter. Further down in the code, if that language doesn't have a matching translation, it'll redirect.
Since this is a tricky area of our codebase I'd prefer to have tests with this merge to ensure no other repercussions and to also show that this change is intentional.
Can you suggest any specific files where extra tests could be added @robhudson ?
Can you suggest any specific files where extra tests could be added @robhudson ?
The TestRender class in lib/l10n_utils/tests/test_base.py would be excellent.
Let me know if you want me to squash the commits
@Ingi-Hong Are you open to adding some tests, too, please? Rob has suggested the file where they would fit:
Can you suggest any specific files where extra tests could be added @robhudson ?
The
TestRenderclass inlib/l10n_utils/tests/test_base.pywould be excellent.
If you need any guidance, let me know
Sure, I'd be happy try. I'll take a look when I get the chance.
Closing as per https://github.com/mozilla/bedrock/issues/5931#issuecomment-2003497640