Implement Redirection for Updated Locale Codes #3023

ayanaar · 2023-11-17T02:51:05Z

This pull request introduces a solution for handling changes to locale codes within our Pontoon application. Updating a locale code (e.g., changing sat-Olck to sat) previously could lead to broken links and inaccessible pages for users who had settings or preferences tied to the old locale code. To address this, I've implemented a redirection mechanism that ensures continuity even when locale codes are changed.

Changes Made:

LocaleCodeHistory Model: Introduced a new model to track the history of changes to locale codes. This model records the old and new codes whenever a locale is updated.
Signals for Locale Updates: Implemented Django signals to capture changes in locale codes and record them in the LocaleCodeHistory model.
Redirection Utility Function (get_locale_or_redirect): Developed a utility function that checks if a requested locale exists. If an entry is found, it either redirects to the view specified by redirect_view_name using the new locale code or returns the Locale object if no redirect_view_name is provided. This function is now used across various views where locales are handled.
Updates to Views: Modified several views, including the settings, translate, team, and localization views, to incorporate the get_locale_or_redirect function. This ensures that any references to outdated locale codes are correctly redirected to the new codes.
Test Cases: Comprehensive test cases have been added to validate the functionality of the get_locale_or_redirect utility function in various views. These tests cover scenarios like code changes, missing codes, and non-existent codes to ensure proper redirection and error handling.

mathjazz · 2023-11-17T08:00:54Z

pontoon/base/models.py

@@ -1563,6 +1563,12 @@ class ProjectSlugHistory(models.Model):
    created_at = models.DateTimeField(auto_now_add=True)


+class LocaleCodeHistory(models.Model):
+    locale = models.ForeignKey("Locale", on_delete=models.CASCADE)
+    old_code = models.CharField(max_length=10)


Locale.code has max_length set to 20, let's use that number here as well.

https://pontoon.mozilla.org/rm-vallader/

- get_locale_or_redirect function now returns a Locale object directly when no redirection is needed, facilitating use of locale names instead of codes. - Adjusted settings view to set the default and custom homepage based on locale names. This change accommodates scenarios where locale codes are altered in the admin panel, ensuring the settings page remains functional. - This approach leverages the stability of locale names, providing resilience against changes in locale codes.”

mathjazz

Thanks for the update!

Left a few notes. It's mostly nits, but we should fix the logic in the settings view.

mathjazz · 2023-11-22T16:30:52Z

pontoon/base/utils.py

+                redirect_url = reverse(redirect_view_name, kwargs=redirect_kwargs)
+                return redirect(redirect_url)
+            else:
+                return code_history.locale  # Return the locale instead of redirecting


Nit: You can dedent the main block like this:

if not redirect_view_name or not url_arg_name: return code_history.locale redirect_kwargs = {url_arg_name: code_history.locale.code} redirect_kwargs.update(kwargs) redirect_url = reverse(redirect_view_name, kwargs=redirect_kwargs) return redirect(redirect_url) ...

Also, no need for the comment, it's already well clarified in the docstring above.

mathjazz · 2023-11-22T16:33:38Z

pontoon/contributors/views.py

-    custom_homepage_locale = profile.custom_homepage
-    if custom_homepage_locale:
+    # Set default for custom homepage locale based on name
+    custom_homepage_locale_object = get_locale_or_redirect(profile.custom_homepage)


I would just leave the name custom_homepage_locale.

Also, you should first check if we have profile.custom_homepage.

If we don't, use default_homepage_locale.
Only if we do, call get_locale_or_redirect(), otherwise it will fail (redirect).

mathjazz · 2023-11-22T16:33:57Z

pontoon/contributors/views.py

-    if custom_homepage_locale:
+    # Set default for custom homepage locale based on name
+    custom_homepage_locale_object = get_locale_or_redirect(profile.custom_homepage)
+    custom_homepage_locale_name = custom_homepage_locale_object.name


Why name (not unique), not code (unique)?

From what I understood from our conversation yesterday, the name of the local rarely would ever be edited (in comparison to the locale code) so I thought that setting the custom and default homepage using the locale name would be more resilient to changes in the locale code. However, if it is not unique then that will cause issues so I will change the logic to use the locale code.

Sorry, sounds like a misunderstanding. Locale names are probably edited more often than locale codes.

When you address the comment above, you might even be good without either.

mathjazz · 2023-11-22T16:47:53Z

pontoon/contributors/views.py

-    preferred_source_locale = profile.preferred_source_locale
-    if preferred_source_locale:
+    # Set preferred source locale based on name
+    preferred_source_locale_object = get_locale_or_redirect(


Same comments as above.

mathjazz

Nice work! Please also add the missing tests as we discussed.

mathjazz · 2023-11-27T12:05:46Z

pontoon/contributors/views.py

        custom_homepage_locale = default_homepage_locale
+        messages.info(
+            request,
+            "Your previously selected custom homepage locale is no longer available. Reverted to default.",


We don't really revert the locale, we just show the default homepage when that happens. The user still needs to press Save. So I'd replace "Reverted to default." with something along the lines of "Please pick a different one.".

mathjazz · 2023-11-27T12:08:12Z

pontoon/contributors/views.py

+        if profile.custom_homepage:
+            custom_homepage_locale = get_locale_or_redirect(profile.custom_homepage)
+        else:
+            custom_homepage_locale = default_homepage_locale


You can move this line in front of the try - except block and then you can remove it from line 321.

At that point you no longer need the else block and you can move the try - except block within the if block.

mathjazz · 2023-11-27T12:08:26Z

pontoon/contributors/views.py

+                profile.preferred_source_locale
+            )
+        else:
+            preferred_source_locale = default_preferred_source_locale


Same as above.

mathjazz · 2023-11-27T12:08:33Z

pontoon/contributors/views.py

        preferred_source_locale = default_preferred_source_locale
+        messages.info(
+            request,
+            "Your previously selected preferred source locale is no longer available. Reverted to default.",


Same as above.

mathjazz

Works great!

I'd just add one thing - if the locale that is set as the custom homepage gets deleted, the homepage 404s.

We can mitigate that by adding a call to get_locale_or_redirect() in front of https://github.com/mozilla/pontoon/blob/main/pontoon/homepage/views.py#L22 and wrap it in a try - except Http404 block (and just pass on exception).

mathjazz

Great job! This is much more than just a bugfix.

I'll file an issue to consider storing custom_homepage as models.ForeignKey(Locale).

ayanaar added 2 commits November 16, 2023 21:36

Adding redirect logic for when locale name is modified

7a039c5

Remove request parameter from get_locale_or_redirect

aef18d4

ayanaar requested a review from mathjazz November 17, 2023 02:51

Satisfy linter

5b06e43

mathjazz reviewed Nov 20, 2023

View reviewed changes

ayanaar added 3 commits November 22, 2023 01:49

Changing max_length in LocaleCodeHistory model

a9f7f74

Satisfy python linter

861cbaf

mathjazz reviewed Nov 22, 2023

View reviewed changes

mathjazz marked this pull request as ready for review November 22, 2023 19:43

ayanaar added 4 commits November 22, 2023 16:23

Refactoring get_locale_or_redirect function

c78209a

Register LocaleCodeHistory model in admin panel

5fd7cf0

Fix logic in the settings view

05e8dbd

Handle case where locale gets completely deleted

4465127

mathjazz reviewed Nov 27, 2023

View reviewed changes

ayanaar added 2 commits November 27, 2023 15:42

Refactor settings view

ddef4cc

Add tests for locale code handling in various views

2e59468

mathjazz reviewed Nov 28, 2023

View reviewed changes

Add check to prevent 404 on homepage when custom locale is deleted

a1e319e

mathjazz approved these changes Nov 28, 2023

View reviewed changes

mathjazz merged commit c41639d into mozilla:main Nov 28, 2023
4 checks passed

mathjazz mentioned this pull request Nov 28, 2023

Consider storing UserProfile.custom_homepage as ForeignKey(Locale) #3034

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Redirection for Updated Locale Codes #3023

Implement Redirection for Updated Locale Codes #3023

ayanaar commented Nov 17, 2023 •

edited

Loading

mathjazz Nov 17, 2023

mathjazz left a comment

mathjazz Nov 22, 2023

mathjazz Nov 22, 2023

mathjazz Nov 22, 2023

mathjazz Nov 22, 2023

ayanaar Nov 22, 2023

mathjazz Nov 22, 2023

mathjazz Nov 22, 2023

mathjazz left a comment

mathjazz Nov 27, 2023

mathjazz Nov 27, 2023

mathjazz Nov 27, 2023

mathjazz Nov 27, 2023

mathjazz left a comment

mathjazz left a comment

Implement Redirection for Updated Locale Codes #3023

Implement Redirection for Updated Locale Codes #3023

Conversation

ayanaar commented Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

mathjazz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathjazz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathjazz left a comment

Choose a reason for hiding this comment

mathjazz left a comment

Choose a reason for hiding this comment

ayanaar commented Nov 17, 2023 •

edited

Loading