Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle Project URL Slug Changes #2889

Merged
merged 12 commits into from
Jul 20, 2023
39 changes: 39 additions & 0 deletions pontoon/base/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,45 @@ def test_project_view_redirects_old_slug(client, project_d):
)


@pytest.mark.django_db
def test_handle_old_slug_redirect_no_loop(client, project_d):
"""
Test that there is no redirect loop when a project's slug is renamed from 'cc' to 'dd' and then back to 'cc'.
"""
# Rename project from 'cc' to 'dd' and then back to 'cc'
create_slug_history_and_change_slug(project_d, "cc")
create_slug_history_and_change_slug(project_d, "dd")
create_slug_history_and_change_slug(project_d, "cc")

# Request the project detail view with slug 'cc'
response = client.get(reverse("pontoon.projects.project", kwargs={"slug": "cc"}))

# Assert that the response is not a redirect (status code is not 302)
assert response.status_code != 302


@pytest.mark.django_db
def test_handle_old_slug_redirect_no_redirect_to_different_project(client, project_d):
"""
Test that a request for a slug that was changed and then reused by a different project does not redirect to the original project.
"""
# Rename project from 'ee' to 'ff'
create_slug_history_and_change_slug(project_d, "ee")
create_slug_history_and_change_slug(project_d, "ff")

# Create a new project with slug 'ee'
project = ProjectFactory.create(
name="Project E", slug="ee", disabled=False, system_project=False
)
ResourceFactory.create(project=project, path="resource_e.po", format="po")

# Request the project detail view with slug 'ee'
response = client.get(reverse("pontoon.projects.project", kwargs={"slug": "ee"}))

# Assert that the response is successful (status code is 200)
assert response.status_code == 200


@pytest.mark.django_db
def test_handle_no_slug_redirect_project(client):
"""
Expand Down
59 changes: 34 additions & 25 deletions pontoon/base/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -616,9 +616,12 @@ def handle_old_slug_redirect(redirect_view, redirect_kwargs):
Decorator for Django view functions that redirects requests with outdated project slugs
to the corresponding current slug URL.

This decorator fetches the ProjectSlugHistory model, and if an old slug is used in a request,
it retrieves the associated current slug and redirects to the corresponding URL.
If no corresponding old slug is found in the history, the original view function is called without redirection.
This decorator first attempts to find a Project using the current slug. If the Project is found,
the original view function is called without redirection.

If a Project is not found with the current slug, the decorator fetches the ProjectSlugHistory model,
and if an old slug is used in a request, it retrieves the associated current slug and redirects to the
corresponding URL. If no corresponding old slug is found in the history, the original view function is called without redirection.

Parameters:
redirect_view (str): The name of the view to which the request should be redirected in case of an old slug.
Expand All @@ -631,6 +634,7 @@ def handle_old_slug_redirect(redirect_view, redirect_kwargs):

def decorator(view_func):
def wrapper(request, *args, **kwargs):
Project = apps.get_model("base", "Project")
ProjectSlugHistory = apps.get_model("base", "ProjectSlugHistory")

# Create a dictionary with the arguments
Expand All @@ -642,29 +646,34 @@ def wrapper(request, *args, **kwargs):
# If slug is still not defined, raise an error.
if slug is None:
raise ValueError("Cannot extract slug from view arguments")
# 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

except Project.DoesNotExist:
# Try to find a project using old slug
slug_history = (
ProjectSlugHistory.objects.filter(old_slug=slug)
.order_by("-created_at")
.first()
)

# Try to find a project using old slug
slug_history = (
ProjectSlugHistory.objects.filter(old_slug=slug)
.order_by("-created_at")
.first()
)

if slug_history is not None:
# Create a dict with the required kwargs for the redirect
redirect_args = {
arg: arguments[arg] if arg in arguments else None
for arg in redirect_kwargs
}
# Replace the slug with the current one
redirect_args[
"slug" if "slug" in redirect_kwargs else "project"
] = slug_history.project.slug

# Use the created dict in the reverse call
redirect_url = reverse(redirect_view, kwargs=redirect_args)

return redirect(redirect_url)
if slug_history is not None:
# Create a dict with the required kwargs for the redirect
redirect_args = {
arg: arguments[arg] if arg in arguments else None
for arg in redirect_kwargs
}
# Replace the slug with the current one
redirect_args[
"slug" if "slug" in redirect_kwargs else "project"
] = slug_history.project.slug

# Use the created dict in the reverse call
redirect_url = reverse(redirect_view, kwargs=redirect_args)

return redirect(redirect_url)

# If no slug history is found, continue with the original view function
return view_func(request, *args, **kwargs)
Expand Down