Handle Project URL Slug Changes #2889

ayanaar · 2023-06-21T18:57:38Z

Fixes #2152

This pull request addresses the issue of properly redirecting old project slugs to the current ones after a project's slug has been updated. The previous approach using a decorator was refactored into a utility function for more efficient handling of outdated slugs. Below are the changes made:

ProjectSlugHistory Model: A new model called ProjectSlugHistory was created to keep a history of old slugs for each project. This model has fields for the project (a foreign key to the Project model), the old slug, and the timestamp of the change.
Pre-Save Signal: The signal create_slug_history was implemented to detect when a project's slug is updated. When a change is detected, a new ProjectSlugHistory instance is created, effectively saving the old slug.
get_project_or_redirect Utility Function: This function attempts to get a project with the given slug. If the project doesn't exist, it checks if the slug is in the ProjectSlugHistory and if so, it redirects to the current project slug URL. If the old slug is not found in the history, it raises an Http404 error.
Utility Function Usage: Several views (e.g., localization, translate, project) have been updated to use the new utility function.
Test Cases: Comprehensive test cases have been added to validate the functionality of the get_project_or_redirect utility function. This includes scenarios with old slugs, missing slugs, and nonexistent slugs for the project, translate, and localization views. These tests ensure the correctness of the redirection process.

This new approach ensures a smooth user experience when slugs are changed, as old links will continue to function correctly by redirecting to the new location. The newly added tests and updated docstrings also enhance code maintainability and clarity.

mathjazz

Nice work!

There's two minor changes I requested, as well as a more holistic approach to redirecting several URLs.

mathjazz · 2023-06-22T11:45:40Z

pontoon/base/admin.py

@@ -292,6 +292,10 @@ class TranslationAdmin(admin.ModelAdmin):
    raw_id_fields = ("entity",)


+class ProjectSlugHistoryAdmin(admin.ModelAdmin):
+    raw_id_fields = ("project",)


This line is not needed, it would be nice however to show the created_at field in the form.

mathjazz · 2023-06-22T11:47:29Z

pontoon/base/models.py

+    created_at = models.DateTimeField(auto_now_add=True)
+
+
+@receiver(pre_save, sender=Project)


Could you please move the signal to the signals.py file?

mathjazz · 2023-06-22T12:08:51Z

pontoon/projects/views.py

+            # Redirect to the current project slug
+            return redirect("pontoon.projects.project", slug=slug_history.project.slug)
+        else:
+            raise Http404("Project not found.")


There are quite a few views in which we'd need a check like this. For example, we'd also want to redirect the localization dashboard (/{locale}/{project}), translate view (/{locale}/{project}/all-resources/), as well as several AJAX views spread across multiple urls.py files.

Can we instead introduce a few URL patters that will catch old project slugs as proposed initially?

…rator

…ug redirection logic and enhanced docstrings

eemeli

I'm a bit concerned with the proposed order of operations, for two reasons:

This adds a ProjectSlugHistory lookup to every request, even when the slug is the current one used by a project. This seems rather wasteful.
What happens if a slug is re-used? I can think of two potentially problematic scenarios:
1. If a project's slug is renamed aa → bb → aa, there's potential redirect loop that may be entered.
2. If one project's slug is renamed aa → bb, and then a different project starts using the aa slug, requests for it get redirected to the bb project.

Both of the above concerns would be resolved if the ProjectSlugHistory lookup were only done after an initial project slug lookup failed. Is there a reason this would not work?

Test cases should also be included for the scenarios mentioned above.

eemeli · 2023-07-12T08:53:43Z

pontoon/base/utils.py

+            # Try to find a project using the current slug
+            try:
+                Project.objects.get(slug=slug)
+                # If found, continue with the original view function
+                return view_func(request, *args, **kwargs)


This is better, but still adds a project lookup to each request that's then discarded. Is there a way to do as I suggested earlier, and to include this redirection in the error handling rather than proactively?

In each of the three functions that use this decorator, there's a call to get_object_or_404() for something like Project.objects.get(slug=slug). Could that call be changed to trigger this flow, instead of the current decorator approach?

…unction get_project_or_redirect

eemeli

This looks good! Some questions and nitpicks on line comments.

@mathjazz Do you want to look at this again, as its shape has evolved somewhat, or should we just merge & push to staging?

pontoon/base/utils.py

mathjazz · 2023-07-17T14:25:13Z

This looks good, nice work!

I'm pushing the branch to stage now.

The only thing I'm left wondering about is whether we should redirect just the three main URLs (current state), or all.

mathjazz · 2023-07-18T18:38:34Z

@flodolo Could you take this for a spin on stage and provide some feedback here?

Essentially you should go to Admin, change slug for a project (or projects) and then test if the app behaves as expected.

flodolo · 2023-07-19T06:39:55Z

This seems to work correctly, although there's an edge case which is not considered, and to me it feels like a bug.

I changed the slug for AMO from amo to amo1, then to amo2. There's a check preventing me to use a slug already in use, which is good (BTW, the message Project with this Slug already exists. feels a bit weird with an article at the beginning, i.e. A project with this Slug already exists.). Links with both amo and amo1 correctly redirect to amo2.

Then I went to a different project (AMO Linter), and switched its slug to amo1. Surprisingly, it let me do that. I assumed we should prevent reusing an existing slug, even if it's not actively used by a project?

mathjazz · 2023-07-19T09:13:43Z

@ayanaar Let's extend slug validation to also prevent the use of historic slugs.

eemeli · 2023-07-19T09:36:15Z

Then I went to a different project (AMO Linter), and switched its slug to amo1. Surprisingly, it let me do that. I assumed we should prevent reusing an existing slug, even if it's not actively used by a project?

This should be fine, and already handled by this PR. Historical slugs are not required to be unique, but on access the most recent slug wins.

For an example of a different system that works this way, see GitHub repo renames: They'll redirect old URLs until and unless you define a new repo with the same name as a past one had.

In my opinion, no further work is required on this PR before finally merging it.

mathjazz · 2023-07-20T14:47:06Z

On Pontoon Call we've decided to move on with the current state of the patch. Nice work, @ayanaar!

ayanaar changed the title ~~Add Slug History and Redirect for Project Model~~ Handle Project URL Slug Changes Jun 21, 2023

ayanaar marked this pull request as ready for review June 21, 2023 21:24

ayanaar requested a review from mathjazz June 21, 2023 21:24

mathjazz reviewed Jun 22, 2023

View reviewed changes

ayanaar marked this pull request as draft June 26, 2023 14:54

ayanaar marked this pull request as ready for review June 27, 2023 16:12

ayanaar added 6 commits July 6, 2023 16:17

Add Slug History and Redirect for Project Model

8c47071

Handle project URL slug changes in project view

d951543

Satisfying linters

188fc09

Modified structure and decorator

e3b181e

Satisfying linters

6dcf9a4

Updated decorator to pass the list of kwargs needed for the redirect

49058f0

ayanaar force-pushed the 2152 branch from 1d55c88 to 49058f0 Compare July 6, 2023 21:01

ayanaar added 2 commits July 6, 2023 17:28

Adding tests for the Project view and the Translate view for the deco…

e40e496

…rator

Linters

09dfd1e

ayanaar marked this pull request as draft July 6, 2023 23:02

Added new test cases for Project, Translate, and Localization view sl…

efbdc96

…ug redirection logic and enhanced docstrings

ayanaar marked this pull request as ready for review July 7, 2023 19:52

ayanaar requested a review from eemeli July 7, 2023 19:52

eemeli requested changes Jul 10, 2023

View reviewed changes

Optimize project slug history lookup and handle slug reuse scenarios

b52c0f2

eemeli requested changes Jul 12, 2023

View reviewed changes

Refactored project slug redirection logic from decorator to utility f…

a93c517

…unction get_project_or_redirect

eemeli approved these changes Jul 17, 2023

View reviewed changes

pontoon/base/utils.py Outdated Show resolved Hide resolved

pontoon/base/utils.py Outdated Show resolved Hide resolved

pontoon/base/utils.py Outdated Show resolved Hide resolved

Refactor imports and replace get_object_or_404 with standard try/except

4326578

mathjazz requested review from mathjazz and eemeli July 18, 2023 14:32

eemeli approved these changes Jul 18, 2023

View reviewed changes

mathjazz merged commit d913a04 into mozilla:master Jul 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle Project URL Slug Changes #2889

Handle Project URL Slug Changes #2889

ayanaar commented Jun 21, 2023 •

edited

Loading

mathjazz left a comment

mathjazz Jun 22, 2023

mathjazz Jun 22, 2023

mathjazz Jun 22, 2023

eemeli left a comment

eemeli Jul 12, 2023

eemeli left a comment

mathjazz commented Jul 17, 2023 •

edited

Loading

mathjazz commented Jul 18, 2023

flodolo commented Jul 19, 2023

mathjazz commented Jul 19, 2023

eemeli commented Jul 19, 2023

mathjazz commented Jul 20, 2023

		created_at = models.DateTimeField(auto_now_add=True)


		@receiver(pre_save, sender=Project)

Handle Project URL Slug Changes #2889

Handle Project URL Slug Changes #2889

Conversation

ayanaar commented Jun 21, 2023 • edited Loading

mathjazz left a comment

Choose a reason for hiding this comment

mathjazz Jun 22, 2023

Choose a reason for hiding this comment

mathjazz Jun 22, 2023

Choose a reason for hiding this comment

mathjazz Jun 22, 2023

Choose a reason for hiding this comment

eemeli left a comment

Choose a reason for hiding this comment

eemeli Jul 12, 2023

Choose a reason for hiding this comment

eemeli left a comment

Choose a reason for hiding this comment

mathjazz commented Jul 17, 2023 • edited Loading

mathjazz commented Jul 18, 2023

flodolo commented Jul 19, 2023

mathjazz commented Jul 19, 2023

eemeli commented Jul 19, 2023

mathjazz commented Jul 20, 2023

ayanaar commented Jun 21, 2023 •

edited

Loading

mathjazz commented Jul 17, 2023 •

edited

Loading