Simplify language chooser #328

julianwachholz · 2023-10-20T07:26:28Z

name: Pull request
about: Submit a pull request for this project
assignees: fabiocaccamo

Describe your changes

Allows usage of i18n_patterns with prefix_default_language=False
Change templatetag from simple tag to an inclusion tag
Reduces complexity by relying on Django's behavior in the set_language view: it will translate any url passed as 'next'. This behavior has been present since Django 1.9.
Remove individual forms for each language

Reference: django/django@aa5ab11

Related issue
#327

Checklist before requesting a review

I have performed a self-review of my code.
I have added tests for the proposed changes.
I have run the tests and there are not errors.

- Allows usage of i18n_patterns with prefix_default_language=False - Change templatetag from simple tag to an inclusion tag - Reduces complexity by relying on Django's behavior in the set_language view: it will translate any url passed as 'next'. This behavior has been present since Django 1.9. - Remove individual forms for each language Fixes fabiocaccamo#327 Reference: django/django@aa5ab11

codecov · 2023-10-20T07:34:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1c1945c) 97.34% compared to head (266810f) 97.34%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #328   +/-   ##
=======================================
  Coverage   97.34%   97.34%           
=======================================
  Files          38       38           
  Lines         414      414           
=======================================
  Hits          403      403           
  Misses         11       11

Flag	Coverage Δ
unittests	`97.34% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

fabiocaccamo · 2023-10-20T18:53:29Z

@julianwachholz thank you very much for this cool PR, I will test an review it as soon as possible!

fabiocaccamo · 2023-11-01T11:53:09Z

@julianwachholz sorry for the late answer, I finally had some time for testing/reviewing this PR.

You made some great optimizations, so it's my intention to merge this PR.

However, the PR doesn't solve #327, if I don't prefix default language (prefix_default_language=False), when I switch to the default language using the language chooser, I land to the current url that still includes the default language prefix (http://localhost:8000/en-us/admin/ instead of http://localhost:8000/admin/) (wrong) and the resulting page is a 404 error page (correct).

At this point, considering that your refactoring looks right to me, it seems to be more a Django set_language bug.

I look forward to your feedback.

fabiocaccamo · 2023-11-06T23:02:47Z

@julianwachholz any update?

julianwachholz · 2023-11-07T06:57:02Z

@julianwachholz any update?

I will look at it soon but I'm on vacation this week. 🙂

julianwachholz · 2023-11-14T23:07:46Z

@fabiocaccamo after retracing your steps I found the culprit: the set_language view needs the LocaleMiddleware installed when using it like this. So I added an additional check for it.

fabiocaccamo · 2023-11-15T09:21:27Z

@julianwachholz the check you added is very useful indeed, thanks!

Unfortunately, this is not the cause of the problem I reported, because I already have django.middleware.locale.LocaleMiddleware in my demo.zip settings.MIDDLEWARE list.

fabiocaccamo

With the check you introduced, I expect to see all tests to fail because there is not django.middleware.locale.LocaleMiddleware in tests.settings.MIDDLEWARE, but some tests succeeded anyway.

julianwachholz · 2023-11-16T10:22:32Z

Thank you for the demo.zip, @fabiocaccamo. I checked it out and wasn't able to produce an error using it. Both with prefix_default_language=True and prefix_default_language=False.

The tests for Django <= 3.1 are passing because the package doesn't have a default_app_config in __init__.py (automatic AppConfig discovery was added in 3.2). So I think this is a different bug related to the Django versions supported by django-admin-interface.

Because the language picker is optional, I removed the check and it will now fail silently on misconfiguration and require users to set it up correctly as the documentation describes.
As a small addition I added a runtime warning that only shows up when the language chooser is enabled.

merwok · 2023-11-16T15:37:59Z

admin_interface/templatetags/admin_interface_tags.py

+        warnings.warn(
+            "Language chooser requires 'django.middleware.locale.LocaleMiddleware' "
+            "in your MIDDLEWARE to work.",
+            stacklevel=1,


Isn’t 1 the default value? Is it correct here? (to determine: check what file and line of code is shown as the origin of the warning in the output)

Yes it's the default. The linter was complaining it wants it explicitly.

Two notes:

that sounds like a bad linter, or strange configuration

the point is that stacklevel=1 is very rarely useful, it’s more common to see stacklevel=2 so that the user code with the problem is shown in the traceback

fabiocaccamo · 2023-11-16T17:05:11Z

However, the PR doesn't solve #327, if I don't prefix default language (prefix_default_language=False), when I switch to the default language using the language chooser, I land to the current url that still includes the default language prefix (http://localhost:8000/en-us/admin/ instead of http://localhost:8000/admin/) (wrong) and the resulting page is a 404 error page (correct).

...
Unfortunately, this is not the cause of the problem I reported, because I already have django.middleware.locale.LocaleMiddleware in my demo.zip settings.MIDDLEWARE list.

@merwok have you the possibility to double-check this?

julianwachholz · 2023-12-04T14:18:33Z

In the meantime, I've come across a bug in the current version where it would redirect me to a URL like /en-gb/en-gb/.... This is fixed in my branch which I'm currently using in our production system.

fabiocaccamo · 2023-12-05T08:29:55Z

@julianwachholz how does it happen? Could you submit a PR with test and bugfix for this too?

julianwachholz · 2023-12-05T08:40:37Z

I haven't been able to reproduce it with my changes. Fixing it in main doesn't seem useful to me if it would be overridden with the changes from this PR?

fabiocaccamo · 2023-12-05T10:49:55Z

Can't you fix it in this PR?

julianwachholz · 2023-12-05T11:09:06Z

It is fixed in this PR

fabiocaccamo · 2023-12-05T11:16:25Z

ah ok sorry, I didn't understand, I thought it was a new bug.

merwok · 2024-01-04T17:42:22Z

Sorry I could not test or answer when you asked Fabio – my projects all use LocalMiddleware so I wasn’t in a state to reproduce the problem noted.

fabiocaccamo · 2024-01-06T11:35:54Z

@merwok no problem, don't worry.

julianwachholz marked this pull request as ready for review October 20, 2023 07:29

fabiocaccamo self-requested a review November 1, 2023 11:51

fabiocaccamo assigned julianwachholz Nov 1, 2023

fabiocaccamo added bug Something isn't working enhancement New feature or request labels Nov 1, 2023

julianwachholz added 2 commits November 14, 2023 23:47

Merge branch 'fabiocaccamo:main' into feat/simple-language-chooser

ae9f71a

Add check to ensure LocaleMiddleware

ba48e68

fabiocaccamo requested changes Nov 15, 2023

View reviewed changes

Remove check in favor of silent warning

70a521d

Fix template tag tests

266810f

merwok reviewed Nov 16, 2023

View reviewed changes

fabiocaccamo merged commit b393c11 into fabiocaccamo:main Dec 5, 2023
45 of 46 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify language chooser #328

Simplify language chooser #328

julianwachholz commented Oct 20, 2023

codecov bot commented Oct 20, 2023 •

edited

fabiocaccamo commented Oct 20, 2023

fabiocaccamo commented Nov 1, 2023 •

edited

fabiocaccamo commented Nov 6, 2023

julianwachholz commented Nov 7, 2023

julianwachholz commented Nov 14, 2023

fabiocaccamo commented Nov 15, 2023 •

edited

fabiocaccamo left a comment

julianwachholz commented Nov 16, 2023

merwok Nov 16, 2023

julianwachholz Nov 16, 2023

merwok Jan 4, 2024

fabiocaccamo commented Nov 16, 2023

julianwachholz commented Dec 4, 2023

fabiocaccamo commented Dec 5, 2023

julianwachholz commented Dec 5, 2023

fabiocaccamo commented Dec 5, 2023

julianwachholz commented Dec 5, 2023

fabiocaccamo commented Dec 5, 2023

merwok commented Jan 4, 2024 •

edited

fabiocaccamo commented Jan 6, 2024

Simplify language chooser #328

Simplify language chooser #328

Conversation

julianwachholz commented Oct 20, 2023

codecov bot commented Oct 20, 2023 • edited

Codecov Report

fabiocaccamo commented Oct 20, 2023

fabiocaccamo commented Nov 1, 2023 • edited

fabiocaccamo commented Nov 6, 2023

julianwachholz commented Nov 7, 2023

julianwachholz commented Nov 14, 2023

fabiocaccamo commented Nov 15, 2023 • edited

fabiocaccamo left a comment

Choose a reason for hiding this comment

julianwachholz commented Nov 16, 2023

merwok Nov 16, 2023

Choose a reason for hiding this comment

julianwachholz Nov 16, 2023

Choose a reason for hiding this comment

merwok Jan 4, 2024

Choose a reason for hiding this comment

fabiocaccamo commented Nov 16, 2023

julianwachholz commented Dec 4, 2023

fabiocaccamo commented Dec 5, 2023

julianwachholz commented Dec 5, 2023

fabiocaccamo commented Dec 5, 2023

julianwachholz commented Dec 5, 2023

fabiocaccamo commented Dec 5, 2023

merwok commented Jan 4, 2024 • edited

fabiocaccamo commented Jan 6, 2024

codecov bot commented Oct 20, 2023 •

edited

fabiocaccamo commented Nov 1, 2023 •

edited

fabiocaccamo commented Nov 15, 2023 •

edited

merwok commented Jan 4, 2024 •

edited