diff --git a/readthedocs/projects/forms.py b/readthedocs/projects/forms.py index 5628efe9f36..075c3c3fa58 100644 --- a/readthedocs/projects/forms.py +++ b/readthedocs/projects/forms.py @@ -669,9 +669,11 @@ class RedirectForm(forms.ModelForm): """Form for project redirects.""" + project = forms.CharField(widget=forms.HiddenInput(), required=False, disabled=True) + class Meta: model = Redirect - fields = ["redirect_type", "from_url", "to_url", "force"] + fields = ["project", "redirect_type", "from_url", "to_url", "force"] def __init__(self, *args, **kwargs): self.project = kwargs.pop('project', None) @@ -685,18 +687,8 @@ def __init__(self, *args, **kwargs): else: self.fields.pop("force") - def save(self, **_): # pylint: disable=arguments-differ - # TODO this should respect the unused argument `commit`. It's not clear - # why this needs to be a call to `create`, instead of relying on the - # super `save()` call. - redirect = Redirect.objects.create( - project=self.project, - redirect_type=self.cleaned_data["redirect_type"], - from_url=self.cleaned_data["from_url"], - to_url=self.cleaned_data["to_url"], - force=self.cleaned_data.get("force", False), - ) - return redirect + def clean_project(self): + return self.project class DomainBaseForm(forms.ModelForm): diff --git a/readthedocs/projects/urls/private.py b/readthedocs/projects/urls/private.py index 1d413ba758b..06469f6812b 100644 --- a/readthedocs/projects/urls/private.py +++ b/readthedocs/projects/urls/private.py @@ -32,8 +32,10 @@ ProjectDelete, ProjectNotifications, ProjectNotificationsDelete, - ProjectRedirects, + ProjectRedirectsCreate, ProjectRedirectsDelete, + ProjectRedirectsList, + ProjectRedirectsUpdate, ProjectTranslationsDelete, ProjectTranslationsListAndCreate, ProjectUpdate, @@ -125,11 +127,21 @@ ), re_path( r'^(?P[-\w]+)/redirects/$', - ProjectRedirects.as_view(), + ProjectRedirectsList.as_view(), name='projects_redirects', ), re_path( - r'^(?P[-\w]+)/redirects/delete/$', + r"^(?P[-\w]+)/redirects/create/$", + ProjectRedirectsCreate.as_view(), + name="projects_redirects_create", + ), + re_path( + r"^(?P[-\w]+)/redirects/(?P[-\w]+)/edit/$", + ProjectRedirectsUpdate.as_view(), + name="projects_redirects_edit", + ), + re_path( + r"^(?P[-\w]+)/redirects/(?P[-\w]+)/delete/$", ProjectRedirectsDelete.as_view(), name='projects_redirects_delete', ), diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 2364510956e..c39e201c1c5 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -712,44 +712,40 @@ class ProjectRedirectsMixin(ProjectAdminMixin, PrivateViewMixin): """Project redirects view and form view.""" + form_class = RedirectForm + template_name = "redirects/redirect_form.html" + context_object_name = "redirect" + lookup_url_kwarg = "redirect_pk" + def get_success_url(self): return reverse( 'projects_redirects', args=[self.get_project().slug], ) + def get_queryset(self): + return self.get_project().redirects.all() -class ProjectRedirects(ProjectRedirectsMixin, FormView): - form_class = RedirectForm - template_name = 'projects/project_redirects.html' +class ProjectRedirectsList(ProjectRedirectsMixin, ListView): - def form_valid(self, form): - form.save() - return HttpResponseRedirect(self.get_success_url()) + template_name = "redirects/redirect_list.html" + context_object_name = "redirects" - def get_context_data(self, **kwargs): - context = super().get_context_data(**kwargs) - project = self.get_project() - context['redirects'] = project.redirects.all() - return context +class ProjectRedirectsCreate(ProjectRedirectsMixin, CreateView): + + pass -class ProjectRedirectsDelete(ProjectRedirectsMixin, GenericView): - http_method_names = ['post'] +class ProjectRedirectsUpdate(ProjectRedirectsMixin, UpdateView): - def post(self, request, *args, **kwargs): - project = self.get_project() - redirect = get_object_or_404( - project.redirects, - pk=request.POST.get('id_pk'), - ) - if redirect.project == project: - redirect.delete() - else: - raise Http404 - return HttpResponseRedirect(self.get_success_url()) + pass + + +class ProjectRedirectsDelete(ProjectRedirectsMixin, DeleteView): + + http_method_names = ['post'] class DomainMixin(ProjectAdminMixin, PrivateViewMixin): diff --git a/readthedocs/templates/projects/project_redirects.html b/readthedocs/redirects/templates/redirects/redirect_form.html similarity index 51% rename from readthedocs/templates/projects/project_redirects.html rename to readthedocs/redirects/templates/redirects/redirect_form.html index 449853c3723..c066fae59c6 100644 --- a/readthedocs/templates/projects/project_redirects.html +++ b/readthedocs/redirects/templates/redirects/redirect_form.html @@ -2,7 +2,7 @@ {% load i18n %} -{% block title %}{% trans "Edit Redirects" %}{% endblock %} +{% block title %}{% trans "Redirect" %}{% endblock %} {% block project-redirects-active %}active{% endblock %} {% block project_edit_content_header %}{% trans "Redirects" %}{% endblock %} @@ -10,7 +10,25 @@ {% block extra_scripts %} @@ -76,44 +74,18 @@ {% endblocktrans %}

- {% if redirects|length > 0 %} -

{% trans "Redirects" %}

-
-
-
-
    - {% for redirect in redirects %} -
  • -
    - {{ redirect.get_redirect_type_display }} -
    - {% if redirect.get_from_to_url_display %} -
    - {{ redirect.get_from_to_url_display }} -
    - {% endif %} -
      -
    • -
      - {% csrf_token %} - - -
      -
    • -
    -
  • - {% endfor %} -
-
-
-
- {% endif %} -
{% csrf_token %} {{ form.as_p }}
-

- -

+
+ + {% if redirect %} +
+ {% csrf_token %} + +
+ {% endif %} {% endblock %} diff --git a/readthedocs/redirects/templates/redirects/redirect_list.html b/readthedocs/redirects/templates/redirects/redirect_list.html new file mode 100644 index 00000000000..998125f8931 --- /dev/null +++ b/readthedocs/redirects/templates/redirects/redirect_list.html @@ -0,0 +1,61 @@ +{% extends "projects/project_edit_base.html" %} + +{% load i18n %} + +{% block title %}{% trans "Redirects" %}{% endblock %} + +{% block project-redirects-active %}active{% endblock %} +{% block project_edit_content_header %}{% trans "Redirects" %}{% endblock %} + +{% block project_edit_content %} +

+ {% blocktrans trimmed with docs_url='https://docs.readthedocs.io/page/user-defined-redirects.html' %} + Add redirects for your project. This allows you to fix links to old pages that are 404ing. Learn more. + {% endblocktrans %} +

+ + + +
+
+
+
    + {% for redirect in redirects %} +
  • +
    + {{ redirect.get_redirect_type_display }} +
    + {% if redirect.get_from_to_url_display %} +
    + {{ redirect.get_from_to_url_display }} +
    + {% endif %} + +
  • + {% empty %} +
  • +

    + {% trans "No redirects found." %} +

    +
  • + {% endfor %} +
+
+
+
+{% endblock %} diff --git a/readthedocs/redirects/tests/__init__.py b/readthedocs/redirects/tests/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/readthedocs/redirects/tests/test_views.py b/readthedocs/redirects/tests/test_views.py new file mode 100644 index 00000000000..48ed0b8d002 --- /dev/null +++ b/readthedocs/redirects/tests/test_views.py @@ -0,0 +1,82 @@ +from django.contrib.auth.models import User +from django.test import TestCase +from django.urls import reverse +from django_dynamic_fixture import get + +from readthedocs.projects.models import Project +from readthedocs.redirects.models import Redirect + + +class TestViewsR(TestCase): + def setUp(self): + self.user = get(User) + self.project = get(Project, slug="test", users=[self.user]) + self.redirect = get( + Redirect, + project=self.project, + redirect_type="exact", + from_url="/404.html", + to_url="/en/latest/", + ) + self.client.force_login(self.user) + + def test_create_redirect(self): + self.assertEqual(self.project.redirects.all().count(), 1) + resp = self.client.post( + reverse("projects_redirects_create", args=[self.project.slug]), + data={ + "redirect_type": "page", + "from_url": "/config.html", + "to_url": "/configuration.html", + }, + ) + self.assertEqual(resp.status_code, 302) + self.assertEqual(self.project.redirects.all().count(), 2) + + def test_update_redirect(self): + self.assertEqual(self.project.redirects.all().count(), 1) + resp = self.client.post( + reverse( + "projects_redirects_edit", args=[self.project.slug, self.redirect.pk] + ), + data={ + "redirect_type": "page", + "from_url": "/config.html", + "to_url": "/configuration.html", + }, + ) + self.assertEqual(resp.status_code, 302) + redirect = self.project.redirects.get() + self.assertEqual(redirect.redirect_type, "page") + self.assertEqual(redirect.from_url, "/config.html") + self.assertEqual(redirect.to_url, "/configuration.html") + self.assertEqual(self.project.redirects.all().count(), 1) + + def test_delete_redirect(self): + self.assertEqual(self.project.redirects.all().count(), 1) + resp = self.client.post( + reverse( + "projects_redirects_delete", args=[self.project.slug, self.redirect.pk] + ), + ) + self.assertEqual(resp.status_code, 302) + self.assertEqual(self.project.redirects.all().count(), 0) + + def test_list_redirect(self): + resp = self.client.get( + reverse("projects_redirects", args=[self.project.slug]), + ) + self.assertEqual(resp.status_code, 200) + self.assertContains(resp, self.redirect.from_url) + self.assertContains(resp, self.redirect.to_url) + + def test_get_redirect(self): + resp = self.client.get( + reverse( + "projects_redirects_edit", args=[self.project.slug, self.redirect.pk] + ), + ) + self.assertEqual(resp.status_code, 200) + self.assertContains(resp, self.redirect.redirect_type) + self.assertContains(resp, self.redirect.from_url) + self.assertContains(resp, self.redirect.to_url) diff --git a/readthedocs/rtd_tests/tests/test_privacy_urls.py b/readthedocs/rtd_tests/tests/test_privacy_urls.py index d769590a389..646535edc2e 100644 --- a/readthedocs/rtd_tests/tests/test_privacy_urls.py +++ b/readthedocs/rtd_tests/tests/test_privacy_urls.py @@ -18,12 +18,8 @@ from readthedocs.core.utils.tasks import TaskNoPermission from readthedocs.integrations.models import HttpExchange, Integration from readthedocs.oauth.models import RemoteOrganization, RemoteRepository -from readthedocs.projects.models import ( - Domain, - EnvironmentVariable, - Project, - WebHook, -) +from readthedocs.projects.models import Domain, EnvironmentVariable, Project, WebHook +from readthedocs.redirects.models import Redirect from readthedocs.rtd_tests.utils import create_user @@ -187,24 +183,30 @@ def setUp(self): response_headers={'foo': 'bar'}, status_code=200, ) + self.redirect = get( + Redirect, + project=self.pip, + redirect_type="sphinx_html", + ) self.default_kwargs = { - 'project_slug': self.pip.slug, - 'subproject_slug': self.subproject.slug, - 'version_slug': self.pip.versions.all()[0].slug, - 'filename': 'index.html', - 'type_': 'pdf', - 'tag': self.tag.slug, - 'child_slug': self.subproject.slug, - 'build_pk': self.build.pk, - 'domain_pk': self.domain.pk, - 'integration_pk': self.integration.pk, - 'exchange_pk': self.integration_exchange.pk, - 'environmentvariable_pk': self.environment_variable.pk, - 'automation_rule_pk': self.automation_rule.pk, - 'steps': 1, - 'invalid_project_slug': 'invalid_slug', - 'webhook_pk': self.webhook.pk, - 'webhook_exchange_pk': self.webhook_exchange.pk, + "project_slug": self.pip.slug, + "subproject_slug": self.subproject.slug, + "version_slug": self.pip.versions.all()[0].slug, + "filename": "index.html", + "type_": "pdf", + "tag": self.tag.slug, + "child_slug": self.subproject.slug, + "build_pk": self.build.pk, + "domain_pk": self.domain.pk, + "redirect_pk": self.redirect.pk, + "integration_pk": self.integration.pk, + "exchange_pk": self.integration_exchange.pk, + "environmentvariable_pk": self.environment_variable.pk, + "automation_rule_pk": self.automation_rule.pk, + "steps": 1, + "invalid_project_slug": "invalid_slug", + "webhook_pk": self.webhook.pk, + "webhook_exchange_pk": self.webhook_exchange.pk, } @@ -277,28 +279,31 @@ class PrivateProjectAdminAccessTest(PrivateProjectMixin, TestCase): '/dashboard/pip/subprojects/delete/sub/': {'status_code': 302}, # 405's where we should be POST'ing - '/dashboard/pip/users/delete/': {'status_code': 405}, - '/dashboard/pip/notifications/delete/': {'status_code': 405}, - '/dashboard/pip/redirects/delete/': {'status_code': 405}, - '/dashboard/pip/subprojects/sub/delete/': {'status_code': 405}, - '/dashboard/pip/integrations/sync/': {'status_code': 405}, - '/dashboard/pip/integrations/{integration_id}/sync/': {'status_code': 405}, - '/dashboard/pip/integrations/{integration_id}/delete/': {'status_code': 405}, - '/dashboard/pip/environmentvariables/{environmentvariable_id}/delete/': {'status_code': 405}, - '/dashboard/pip/translations/delete/sub/': {'status_code': 405}, - '/dashboard/pip/version/latest/delete_html/': {'status_code': 405}, - '/dashboard/pip/rules/{automation_rule_id}/delete/': {'status_code': 405}, - '/dashboard/pip/rules/{automation_rule_id}/move/{steps}/': {'status_code': 405}, - '/dashboard/pip/webhooks/{webhook_id}/delete/': {'status_code': 405}, + "/dashboard/pip/users/delete/": {"status_code": 405}, + "/dashboard/pip/notifications/delete/": {"status_code": 405}, + "/dashboard/pip/redirects/{redirect_pk}/delete/": {"status_code": 405}, + "/dashboard/pip/subprojects/sub/delete/": {"status_code": 405}, + "/dashboard/pip/integrations/sync/": {"status_code": 405}, + "/dashboard/pip/integrations/{integration_id}/sync/": {"status_code": 405}, + "/dashboard/pip/integrations/{integration_id}/delete/": {"status_code": 405}, + "/dashboard/pip/environmentvariables/{environmentvariable_id}/delete/": { + "status_code": 405 + }, + "/dashboard/pip/translations/delete/sub/": {"status_code": 405}, + "/dashboard/pip/version/latest/delete_html/": {"status_code": 405}, + "/dashboard/pip/rules/{automation_rule_id}/delete/": {"status_code": 405}, + "/dashboard/pip/rules/{automation_rule_id}/move/{steps}/": {"status_code": 405}, + "/dashboard/pip/webhooks/{webhook_id}/delete/": {"status_code": 405}, } def get_url_path_ctx(self): return { - 'integration_id': self.integration.id, - 'environmentvariable_id': self.environment_variable.id, - 'automation_rule_id': self.automation_rule.id, - 'webhook_id': self.webhook.id, - 'steps': 1, + "integration_id": self.integration.id, + "environmentvariable_id": self.environment_variable.id, + "automation_rule_id": self.automation_rule.id, + "webhook_id": self.webhook.id, + "redirect_pk": self.redirect.pk, + "steps": 1, } def login(self): @@ -322,19 +327,21 @@ class PrivateProjectUserAccessTest(PrivateProjectMixin, TestCase): '/dashboard/pip/': {'status_code': 301}, # 405's where we should be POST'ing - '/dashboard/pip/users/delete/': {'status_code': 405}, - '/dashboard/pip/notifications/delete/': {'status_code': 405}, - '/dashboard/pip/redirects/delete/': {'status_code': 405}, - '/dashboard/pip/subprojects/sub/delete/': {'status_code': 405}, - '/dashboard/pip/integrations/sync/': {'status_code': 405}, - '/dashboard/pip/integrations/{integration_id}/sync/': {'status_code': 405}, - '/dashboard/pip/integrations/{integration_id}/delete/': {'status_code': 405}, - '/dashboard/pip/environmentvariables/{environmentvariable_id}/delete/': {'status_code': 405}, - '/dashboard/pip/translations/delete/sub/': {'status_code': 405}, - '/dashboard/pip/version/latest/delete_html/': {'status_code': 405}, - '/dashboard/pip/rules/{automation_rule_id}/delete/': {'status_code': 405}, - '/dashboard/pip/rules/{automation_rule_id}/move/{steps}/': {'status_code': 405}, - '/dashboard/pip/webhooks/{webhook_id}/delete/': {'status_code': 405}, + "/dashboard/pip/users/delete/": {"status_code": 405}, + "/dashboard/pip/notifications/delete/": {"status_code": 405}, + "/dashboard/pip/redirects/{redirect_pk}/delete/": {"status_code": 405}, + "/dashboard/pip/subprojects/sub/delete/": {"status_code": 405}, + "/dashboard/pip/integrations/sync/": {"status_code": 405}, + "/dashboard/pip/integrations/{integration_id}/sync/": {"status_code": 405}, + "/dashboard/pip/integrations/{integration_id}/delete/": {"status_code": 405}, + "/dashboard/pip/environmentvariables/{environmentvariable_id}/delete/": { + "status_code": 405 + }, + "/dashboard/pip/translations/delete/sub/": {"status_code": 405}, + "/dashboard/pip/version/latest/delete_html/": {"status_code": 405}, + "/dashboard/pip/rules/{automation_rule_id}/delete/": {"status_code": 405}, + "/dashboard/pip/rules/{automation_rule_id}/move/{steps}/": {"status_code": 405}, + "/dashboard/pip/webhooks/{webhook_id}/delete/": {"status_code": 405}, } # Filtered out by queryset on projects that we don't own. @@ -342,11 +349,12 @@ class PrivateProjectUserAccessTest(PrivateProjectMixin, TestCase): def get_url_path_ctx(self): return { - 'integration_id': self.integration.id, - 'environmentvariable_id': self.environment_variable.id, - 'automation_rule_id': self.automation_rule.id, - 'webhook_id': self.webhook.id, - 'steps': 1, + "integration_id": self.integration.id, + "environmentvariable_id": self.environment_variable.id, + "automation_rule_id": self.automation_rule.id, + "webhook_id": self.webhook.id, + "redirect_pk": self.redirect.pk, + "steps": 1, } def login(self): diff --git a/readthedocs/rtd_tests/tests/test_views.py b/readthedocs/rtd_tests/tests/test_views.py index 4e4b202df67..3de44848ab2 100644 --- a/readthedocs/rtd_tests/tests/test_views.py +++ b/readthedocs/rtd_tests/tests/test_views.py @@ -150,7 +150,9 @@ def test_project_redirects(self): self.assertRedirectToLogin(response) def test_project_redirects_delete(self): - response = self.client.get('/dashboard/pip/redirects/delete/') + response = self.client.get( + reverse("projects_redirects_delete", args=["pip", 3]) + ) self.assertRedirectToLogin(response)