From a95db5600637c4b8c37df0359f38bc5514d4580d Mon Sep 17 00:00:00 2001 From: Eric Holscher <25510+ericholscher@users.noreply.github.com> Date: Mon, 15 Jul 2024 08:42:33 -0700 Subject: [PATCH] Refactor admin views to use the SuccessMessageMixin (#11463) * Refactor admin views to use the SuccessMessageMixin This updates most of our views to include success_message on them, and use the SuccessMessageMixin on the base ProjectAdminMixin. This should get us proper dashboard validation for this code now. * Add DeleteViewWithMessage to support delete messages * Add test * Refactor mixin * Add a couple more tests * Fix tests * Fix feedback from review --- readthedocs/core/mixins.py | 22 ++++- readthedocs/organizations/views/private.py | 28 ++---- .../projects/tests/test_domain_views.py | 11 +++ readthedocs/projects/tests/test_views.py | 6 ++ readthedocs/projects/views/base.py | 6 +- readthedocs/projects/views/private.py | 98 ++++++++++++------- 6 files changed, 116 insertions(+), 55 deletions(-) diff --git a/readthedocs/core/mixins.py b/readthedocs/core/mixins.py index 41abca667ca..fb221261bba 100644 --- a/readthedocs/core/mixins.py +++ b/readthedocs/core/mixins.py @@ -1,6 +1,8 @@ """Common mixin classes for views.""" + +from django.contrib import messages from django.contrib.auth.mixins import LoginRequiredMixin -from vanilla import ListView +from vanilla import DeleteView, ListView from readthedocs.proxito.cache import cache_response, private_response @@ -63,3 +65,21 @@ def dispatch(self, request, *args, **kwargs): def can_be_cached(self, request): return self.cache_response + + +class DeleteViewWithMessage(DeleteView): + + """ + Delete view that shows a message after deleting an object. + + Refs https://code.djangoproject.com/ticket/21926 + """ + + success_message = None + + def post(self, request, *args, **kwargs): + resp = super().post(request, *args, **kwargs) + # Check if resp is a redirect, which means the object was deleted + if resp.status_code == 302 and self.success_message: + messages.success(self.request, self.success_message) + return resp diff --git a/readthedocs/organizations/views/private.py b/readthedocs/organizations/views/private.py index ef1c8385059..fef862f57e6 100644 --- a/readthedocs/organizations/views/private.py +++ b/readthedocs/organizations/views/private.py @@ -9,13 +9,13 @@ from django.utils import timezone from django.utils.http import urlencode from django.utils.translation import gettext_lazy as _ -from vanilla import CreateView, DeleteView, FormView, ListView, UpdateView +from vanilla import CreateView, FormView, ListView, UpdateView from readthedocs.audit.filters import OrganizationSecurityLogFilter from readthedocs.audit.models import AuditLog from readthedocs.core.filters import FilterContextMixin from readthedocs.core.history import UpdateChangeReasonPostView -from readthedocs.core.mixins import PrivateViewMixin +from readthedocs.core.mixins import DeleteViewWithMessage, PrivateViewMixin from readthedocs.invitations.models import Invitation from readthedocs.organizations.filters import OrganizationListFilterSet from readthedocs.organizations.forms import ( @@ -123,7 +123,7 @@ class DeleteOrganization( PrivateViewMixin, UpdateChangeReasonPostView, OrganizationView, - DeleteView, + DeleteViewWithMessage, ): template_name = "organizations/admin/organization_delete.html" @@ -151,14 +151,15 @@ def form_valid(self, form): return super().form_valid(form) -class DeleteOrganizationOwner(PrivateViewMixin, OrganizationOwnerView, DeleteView): +class DeleteOrganizationOwner( + PrivateViewMixin, OrganizationOwnerView, DeleteViewWithMessage +): success_message = _("Owner removed") http_method_names = ["post"] def post(self, request, *args, **kwargs): if self._is_last_user(): return HttpResponseBadRequest(_("User is the last owner, can't be removed")) - messages.success(self.request, self.success_message) return super().post(request, *args, **kwargs) @@ -172,17 +173,11 @@ class DeleteOrganizationTeam( PrivateViewMixin, UpdateChangeReasonPostView, OrganizationTeamView, - DeleteView, + DeleteViewWithMessage, ): template_name = "organizations/team_delete.html" success_message = _("Team deleted") - def post(self, request, *args, **kwargs): - """Hack to show messages on delete.""" - resp = super().post(request, *args, **kwargs) - messages.success(self.request, self.success_message) - return resp - def get_success_url(self): return reverse_lazy( "organization_team_list", @@ -203,6 +198,7 @@ class UpdateOrganizationTeamProject(PrivateViewMixin, OrganizationTeamView, Upda class AddOrganizationTeamMember(PrivateViewMixin, OrganizationTeamMemberView, FormView): template_name = "organizations/team_member_create.html" + # No success message here, since it's set in the form. def form_valid(self, form): # Manually calling to save, since this isn't a ModelFormView. @@ -215,17 +211,11 @@ def form_valid(self, form): class DeleteOrganizationTeamMember( - PrivateViewMixin, OrganizationTeamMemberView, DeleteView + PrivateViewMixin, OrganizationTeamMemberView, DeleteViewWithMessage ): success_message = _("Member removed from team") http_method_names = ["post"] - def post(self, request, *args, **kwargs): - """Hack to show messages on delete.""" - resp = super().post(request, *args, **kwargs) - messages.success(self.request, self.success_message) - return resp - class OrganizationSecurityLog(PrivateViewMixin, OrganizationMixin, ListView): diff --git a/readthedocs/projects/tests/test_domain_views.py b/readthedocs/projects/tests/test_domain_views.py index d4f1ef8ac54..a98bfb9c1b1 100644 --- a/readthedocs/projects/tests/test_domain_views.py +++ b/readthedocs/projects/tests/test_domain_views.py @@ -1,4 +1,5 @@ from django.contrib.auth.models import User +from django.contrib.messages import get_messages from django.test import TestCase, override_settings from django.urls import reverse from django_dynamic_fixture import get @@ -34,6 +35,11 @@ def test_domain_creation(self): domain = self.project.domains.first() self.assertEqual(domain.domain, "test.example.com") + # Ensure a message is shown + messages = list(get_messages(resp.wsgi_request)) + self.assertEqual(len(messages), 1) + self.assertEqual(str(messages[0]), "Domain created") + def test_domain_deletion(self): domain = get(Domain, project=self.project, domain="test.example.com") self.assertEqual(self.project.domains.count(), 1) @@ -44,6 +50,11 @@ def test_domain_deletion(self): self.assertEqual(resp.status_code, 302) self.assertEqual(self.project.domains.count(), 0) + # Ensure a message is shown + messages = list(get_messages(resp.wsgi_request)) + self.assertEqual(len(messages), 1) + self.assertEqual(str(messages[0]), "Domain deleted") + def test_domain_edit(self): domain = get( Domain, project=self.project, domain="test.example.com", canonical=False diff --git a/readthedocs/projects/tests/test_views.py b/readthedocs/projects/tests/test_views.py index 1bf46277a59..29b10f383aa 100644 --- a/readthedocs/projects/tests/test_views.py +++ b/readthedocs/projects/tests/test_views.py @@ -1,5 +1,6 @@ from allauth.account.models import EmailAddress from django.contrib.auth.models import User +from django.contrib.messages import get_messages from django.test import TestCase, override_settings from django.urls import reverse from django_dynamic_fixture import get @@ -255,6 +256,11 @@ def test_delete_maintainer(self): self.assertEqual(resp.status_code, 302) self.assertNotIn(self.user, self.project.users.all()) + # Ensure a message is shown + messages = list(get_messages(resp.wsgi_request)) + self.assertEqual(len(messages), 1) + self.assertEqual(str(messages[0]), "User deleted") + def test_delete_last_maintainer(self): url = reverse("projects_users_delete", args=[self.project.slug]) self.client.force_login(self.user) diff --git a/readthedocs/projects/views/base.py b/readthedocs/projects/views/base.py index 711b5840693..6ddd6e9fd75 100644 --- a/readthedocs/projects/views/base.py +++ b/readthedocs/projects/views/base.py @@ -3,6 +3,7 @@ import structlog from django.conf import settings +from django.contrib.messages.views import SuccessMessageMixin from django.shortcuts import get_object_or_404, render from readthedocs.projects.models import Project @@ -40,7 +41,7 @@ def get_context_data(self, **kwargs): # Mixins -class ProjectAdminMixin: +class ProjectAdminMixin(SuccessMessageMixin): """ Mixin class that provides project sublevel objects. @@ -49,6 +50,9 @@ class ProjectAdminMixin: project_url_field The URL kwarg name for the project slug + + success_message + Message when the form is successfully saved, comes from SuccessMessageMixin """ project_url_field = "project_slug" diff --git a/readthedocs/projects/views/private.py b/readthedocs/projects/views/private.py index 89ef0307396..d72719c92b0 100644 --- a/readthedocs/projects/views/private.py +++ b/readthedocs/projects/views/private.py @@ -22,7 +22,6 @@ from formtools.wizard.views import SessionWizardView from vanilla import ( CreateView, - DeleteView, DetailView, FormView, GenericModelView, @@ -40,7 +39,11 @@ ) from readthedocs.core.filters import FilterContextMixin from readthedocs.core.history import UpdateChangeReasonPostView -from readthedocs.core.mixins import ListViewWithForm, PrivateViewMixin +from readthedocs.core.mixins import ( + DeleteViewWithMessage, + ListViewWithForm, + PrivateViewMixin, +) from readthedocs.core.notifications import MESSAGE_EMAIL_VALIDATION_PENDING from readthedocs.core.permissions import AdminPermission from readthedocs.integrations.models import HttpExchange, Integration @@ -102,7 +105,6 @@ class ProjectDashboard(FilterContextMixin, PrivateViewMixin, ListView): model = Project template_name = "projects/project_dashboard.html" - filterset_class = ProjectListFilterSet def get_context_data(self, **kwargs): @@ -175,7 +177,9 @@ def get(self, request, *args, **kwargs): return super().get(self, request, *args, **kwargs) -class ProjectMixin(PrivateViewMixin): +# SuccessMessageMixin is used when we are operating on the Project model itself, +# instead of a related model, where we use ProjectAdminMixin. +class ProjectMixin(SuccessMessageMixin, PrivateViewMixin): """Common pieces for model views of Project.""" @@ -197,16 +201,7 @@ def get_success_url(self): return reverse("projects_detail", args=[self.object.slug]) -class AddonsConfigUpdate(ProjectAdminMixin, PrivateViewMixin, CreateView, UpdateView): - form_class = AddonsConfigForm - success_message = _("Project addons updated") - template_name = "projects/addons_form.html" - - def get_success_url(self): - return reverse("projects_addons", args=[self.object.project.slug]) - - -class ProjectDelete(UpdateChangeReasonPostView, ProjectMixin, DeleteView): +class ProjectDelete(UpdateChangeReasonPostView, ProjectMixin, DeleteViewWithMessage): success_message = _("Project deleted") template_name = "projects/project_delete.html" @@ -219,6 +214,15 @@ def get_success_url(self): return reverse("projects_dashboard") +class AddonsConfigUpdate(ProjectAdminMixin, PrivateViewMixin, CreateView, UpdateView): + form_class = AddonsConfigForm + success_message = _("Project addons updated") + template_name = "projects/addons_form.html" + + def get_success_url(self): + return reverse("projects_addons", args=[self.object.project.slug]) + + class ProjectVersionMixin(ProjectAdminMixin, PrivateViewMixin): model = Version context_object_name = "version" @@ -263,10 +267,12 @@ def form_valid(self, form): class ProjectVersionCreate(ProjectVersionEditMixin, CreateView): + success_message = _("Version created") template_name = "projects/project_version_detail.html" class ProjectVersionDetail(ProjectVersionEditMixin, UpdateView): + success_message = _("Version updated") template_name = "projects/project_version_detail.html" @@ -451,15 +457,16 @@ class ProjectRelationshipList( class ProjectRelationshipCreate(ProjectRelationshipMixin, CreateView): - pass + success_message = _("Subproject created") class ProjectRelationshipUpdate(ProjectRelationshipMixin, UpdateView): - pass + success_message = _("Subproject updated") -class ProjectRelationshipDelete(ProjectRelationshipMixin, DeleteView): +class ProjectRelationshipDelete(ProjectRelationshipMixin, DeleteViewWithMessage): http_method_names = ["post"] + success_message = _("Subproject deleted") class ProjectUsersMixin(ProjectAdminMixin, PrivateViewMixin): @@ -480,7 +487,7 @@ def get_form(self, data=None, files=None, **kwargs): return super().get_form(data, files, **kwargs) -class ProjectUsersList(SuccessMessageMixin, ProjectUsersMixin, FormView): +class ProjectUsersList(ProjectUsersMixin, FormView): # We only use this to display the form in the list view. http_method_names = ["get"] template_name = "projects/project_users.html" @@ -496,12 +503,13 @@ def get_context_data(self, **kwargs): return context -class ProjectUsersCreate(SuccessMessageMixin, ProjectUsersMixin, CreateView): +class ProjectUsersCreate(ProjectUsersMixin, CreateView): success_message = _("Invitation sent") template_name = "projects/project_users_form.html" class ProjectUsersDelete(ProjectUsersMixin, GenericView): + success_message = _("User deleted") http_method_names = ["post"] def post(self, request, *args, **kwargs): @@ -518,6 +526,8 @@ def post(self, request, *args, **kwargs): project = self.get_project() project.users.remove(user) + messages.success(self.request, self.success_message) + if user == request.user: return HttpResponseRedirect(reverse("projects_dashboard")) @@ -575,10 +585,12 @@ def get_context_data(self, **kwargs): class ProjectEmailNotificationsCreate(ProjectNotificationsMixin, CreateView): template_name = "projects/project_notifications_form.html" + success_message = _("Notification created") class ProjectNotificationsDelete(ProjectNotificationsMixin, GenericView): http_method_names = ["post"] + success_message = _("Notification deleted") def post(self, request, *args, **kwargs): project = self.get_project() @@ -613,6 +625,8 @@ class WebHookList(WebHookMixin, ListView): class WebHookCreate(WebHookMixin, CreateView): + success_message = _("Webhook created") + def get_success_url(self): return reverse( "projects_webhooks_edit", @@ -621,6 +635,8 @@ def get_success_url(self): class WebHookUpdate(WebHookMixin, UpdateView): + success_message = _("Webhook updated") + def get_success_url(self): return reverse( "projects_webhooks_edit", @@ -628,7 +644,8 @@ def get_success_url(self): ) -class WebHookDelete(WebHookMixin, DeleteView): +class WebHookDelete(WebHookMixin, DeleteViewWithMessage): + success_message = _("Webhook deleted") http_method_names = ["post"] @@ -683,10 +700,12 @@ def get_context_data(self, **kwargs): class ProjectTranslationsCreate(ProjectTranslationsMixin, CreateView): + success_message = _("Translation created") template_name = "projects/project_translations_form.html" class ProjectTranslationsDelete(ProjectTranslationsMixin, GenericView): + success_message = _("Translation deleted") http_method_names = ["post"] def post(self, request, *args, **kwargs): @@ -729,11 +748,11 @@ class ProjectRedirectsList(ProjectRedirectsMixin, ListView): class ProjectRedirectsCreate(ProjectRedirectsMixin, CreateView): - pass + success_message = _("Redirect created") class ProjectRedirectsUpdate(ProjectRedirectsMixin, UpdateView): - pass + success_message = _("Redirect updated") class ProjectRedirectsInsert(ProjectRedirectsMixin, GenericModelView): @@ -761,8 +780,9 @@ def post(self, request, *args, **kwargs): ) -class ProjectRedirectsDelete(ProjectRedirectsMixin, DeleteView): +class ProjectRedirectsDelete(ProjectRedirectsMixin, DeleteViewWithMessage): http_method_names = ["post"] + success_message = _("Redirect deleted") class DomainMixin(ProjectAdminMixin, PrivateViewMixin): @@ -795,6 +815,8 @@ def get_context_data(self, **kwargs): class DomainCreate(DomainMixin, CreateView): + success_message = _("Domain created") + def post(self, request, *args, **kwargs): project = self.get_project() if self._is_enabled(project) and not project.superproject: @@ -813,6 +835,8 @@ def get_success_url(self): class DomainUpdate(DomainMixin, UpdateView): + success_message = _("Domain updated") + def form_valid(self, form): response = super().form_valid(form) self.object.restart_validation_process() @@ -825,8 +849,8 @@ def post(self, request, *args, **kwargs): return HttpResponse("Action not allowed", status=401) -class DomainDelete(DomainMixin, DeleteView): - pass +class DomainDelete(DomainMixin, DeleteViewWithMessage): + success_message = _("Domain deleted") class IntegrationMixin(ProjectAdminMixin, PrivateViewMixin): @@ -871,6 +895,8 @@ class IntegrationList(IntegrationMixin, ListView): class IntegrationCreate(IntegrationMixin, CreateView): + success_message = _("Integration created") + def form_valid(self, form): self.object = form.save() if self.object.has_sync: @@ -895,7 +921,8 @@ class IntegrationDetail(IntegrationMixin, DetailView): template_name = "projects/integration_webhook_detail.html" -class IntegrationDelete(IntegrationMixin, DeleteView): +class IntegrationDelete(IntegrationMixin, DeleteViewWithMessage): + success_message = _("Integration deleted") http_method_names = ["post"] @@ -940,7 +967,7 @@ def get_success_url(self): return reverse("projects_integrations", args=[self.get_project().slug]) -class ProjectAdvertisingUpdate(PrivateViewMixin, UpdateView): +class ProjectAdvertisingUpdate(SuccessMessageMixin, PrivateViewMixin, UpdateView): model = Project form_class = ProjectAdvertisingForm success_message = _("Project has been opted out from advertisement support") @@ -957,7 +984,7 @@ def get_success_url(self): class EnvironmentVariableMixin(ProjectAdminMixin, PrivateViewMixin): - """Environment Variables to be added when building the Project.""" + """Environment variables to be added when building the Project.""" model = EnvironmentVariable form_class = EnvironmentVariableForm @@ -975,10 +1002,11 @@ class EnvironmentVariableList(EnvironmentVariableMixin, ListView): class EnvironmentVariableCreate(EnvironmentVariableMixin, CreateView): - pass + success_message = _("Environment variable created") -class EnvironmentVariableDelete(EnvironmentVariableMixin, DeleteView): +class EnvironmentVariableDelete(EnvironmentVariableMixin, DeleteViewWithMessage): + success_message = _("Environment variable deleted") http_method_names = ["post"] @@ -1003,6 +1031,7 @@ def get_context_data(self, **kwargs): class AutomationRuleMove(AutomationRuleMixin, GenericModelView): + success_message = _("Automation rule moved") http_method_names = ["post"] def post(self, request, *args, **kwargs): @@ -1017,7 +1046,8 @@ def post(self, request, *args, **kwargs): ) -class AutomationRuleDelete(AutomationRuleMixin, DeleteView): +class AutomationRuleDelete(AutomationRuleMixin, DeleteViewWithMessage): + success_message = _("Automation rule deleted") http_method_names = ["post"] @@ -1027,11 +1057,11 @@ class RegexAutomationRuleMixin(AutomationRuleMixin): class RegexAutomationRuleCreate(RegexAutomationRuleMixin, CreateView): - pass + success_message = _("Automation rule created") class RegexAutomationRuleUpdate(RegexAutomationRuleMixin, UpdateView): - pass + success_message = _("Automation rule updated") class SearchAnalytics(ProjectAdminMixin, PrivateViewMixin, TemplateView): @@ -1216,7 +1246,7 @@ def _get_feature(self, project): return get_feature(project, feature_type=self.feature_type) -class ProjectPullRequestsUpdate(PrivateViewMixin, UpdateView): +class ProjectPullRequestsUpdate(SuccessMessageMixin, PrivateViewMixin, UpdateView): model = Project form_class = ProjectPullRequestForm success_message = _("Pull request settings have been updated")