Support cross-locale slug lookup in language switcher #6634
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the language switcher functionality by adding support for cross-locale slug lookups to prevent 404 errors when document slugs differ between locales. Key changes include:
- Updating the get_visible_document_or_404 logic to search for translations in both default and non-default locales.
- Implementing separate handling for default and non-default locale lookups.
- Adding comprehensive tests to validate direct locale matches, cross-locale lookups, and fallback behaviors.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| kitsune/wiki/utils.py | Updated lookup logic to support cross-locale searches and translation fallback. |
| kitsune/wiki/tests/test_utils.py | Added new tests covering direct matches, cross-locale lookups, fallback scenarios, and access restrictions. |
|
|
||
| # When switching from French to English, we should find the English document | ||
| # even though we're looking for the French slug in English | ||
| doc = get_visible_document_or_404( |
There was a problem hiding this comment.
Isn't this the same assertion with the German document?
There was a problem hiding this comment.
I'm not sure I follow you - all these tests have subtle differences, either slug or locale or other flags. Let me know if I've missed something.
|
I dropped most of the comments from the test file. I left many comments in the |
| if translation: | ||
| return translation | ||
|
|
||
| if not look_for_translation_via_parent or locale == settings.WIKI_DEFAULT_LANGUAGE: |
There was a problem hiding this comment.
Is there a reason this check is so late in the code after all the expensive operations above? This should be combined with the earlier 404.
There was a problem hiding this comment.
Yes - the second check after the loop prevents the final fallback when we're in the default language. Checking it earlier breaks the case where a user requests a document in one locale but is using a slug from another. So - check to see if we should bother finding a translation.
kitsune/wiki/utils.py
Outdated
| if slug and look_for_translation_via_parent: | ||
| # Find documents with this slug in other locales | ||
| other_docs_qs = ( | ||
| Document.objects.filter(slug=slug).exclude(locale=locale).select_related("parent") |
There was a problem hiding this comment.
Shouldn't we use visible here too like above instead of filter?
There was a problem hiding this comment.
My thoughts were that since we are looking for a path to a document through other documents, the documents we look at to move to a valid document should all be collected. We check for visibility later before presenting a final document. What if a document is restricted in one language but not in another?
There was a problem hiding this comment.
That's problematic and it shouldn't happen. The restricted visibility should be the same across all locales
There was a problem hiding this comment.
I still believe this is the right approach - doing discovery of the redirect chain has nothing to do with visibility. If there is a case where a document in the chain is not visible to the user, but the document the user should land on is visible - then they won't get there if we change this to use .visible.
We could wind up with a case where locale leaders or content editors or others with unique permissions will get different redirects than other users.
Unless you can think of a reason that this is harmful in some other way, I'd like to leave it.
The language switcher previously failed when switching between locales where document slugs differ. For example, when switching from German "add-on-badges-abzeichen" back to English "add-on-badges", users would see a 404 error. This commit enhances the get_visible_document_or_404 function to: - Find correct documents across locales with different slugs - Look up documents by finding related translations in any locale - Maintain backward compatibility with existing behavior Added comprehensive tests to verify the new functionality works with: - Direct locale/slug matches - Cross-locale lookups (any locale to any locale) - Document fallbacks when translations don't exist
smithellis
force-pushed
the
locale-fallback-change
branch
from
ca2836a to
615cc6c
Compare
The language switcher previously failed when switching between locales where document slugs differ. For example, when switching from German "add-on-badges-abzeichen" back to English "add-on-badges", users would see a 404 error.
This commit enhances the get_visible_document_or_404 function to:
Added comprehensive tests to verify the new functionality works with: