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

Fix CopyView not prefilling the form data for snippets #11943

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions wagtail/admin/views/generic/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
)
from .models import ( # noqa: F401
CopyView,
CopyViewMixin,
CreateView,
DeleteView,
EditView,
Expand Down
14 changes: 10 additions & 4 deletions wagtail/admin/views/generic/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -688,12 +688,18 @@ def form_invalid(self, form):
return super().form_invalid(form)


class CopyView(CreateView):
class CopyViewMixin:
def get_object(self, queryset=None):
return get_object_or_404(self.model, pk=unquote(str(self.kwargs["pk"])))
return get_object_or_404(
self.model, pk=unquote(str(self.kwargs[self.pk_url_kwarg]))
)
Comment on lines +691 to +695
Copy link
Member Author

@laymonage laymonage May 10, 2024

Choose a reason for hiding this comment

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

Extract this into a mixin so the snippets view can just mix this into its CreateView.

Would be nice to unify this along with how we do our "detail" views, which currently does this in different ways...

Comparison

EditView falls back into the first positional arg if the URL conf does not provide the pk_url_kwarg:

def get_object(self, queryset=None):
if self.pk_url_kwarg not in self.kwargs:
self.kwargs[self.pk_url_kwarg] = self.args[0]
self.kwargs[self.pk_url_kwarg] = unquote(str(self.kwargs[self.pk_url_kwarg]))
return super().get_object(queryset)

DeleteView has an optimisation since it loads the object so soon in setup():

def get_object(self, queryset=None):
# If the object has already been loaded, return it to avoid another query
if getattr(self, "object", None):
return self.object
if self.pk_url_kwarg not in self.kwargs:
self.kwargs[self.pk_url_kwarg] = self.args[0]
self.kwargs[self.pk_url_kwarg] = unquote(str(self.kwargs[self.pk_url_kwarg]))
return super().get_object(queryset)

InspectView:

def setup(self, request, *args, **kwargs):
super().setup(request, *args, **kwargs)
self.pk = self.kwargs[self.pk_url_kwarg]
self.fields = self.get_fields()
self.object = self.get_object()
def get_object(self, queryset=None):
return get_object_or_404(self.model, pk=unquote(str(self.pk)))

RevisionCompareView:

def setup(self, request, pk, revision_id_a, revision_id_b, *args, **kwargs):
super().setup(request, *args, **kwargs)
self.pk = pk
self.revision_id_a = revision_id_a
self.revision_id_b = revision_id_b
self.object = self.get_object()
def get_object(self, queryset=None):
return get_object_or_404(self.model, pk=unquote(str(self.pk)))

UnpublishView:

def setup(self, request, pk, *args, **kwargs):
super().setup(request, *args, **kwargs)
self.pk = pk
self.object = self.get_object()
def dispatch(self, request, *args, **kwargs):
self.objects_to_unpublish = self.get_objects_to_unpublish()
return super().dispatch(request, *args, **kwargs)
def get_object(self, queryset=None):
if not self.model or not issubclass(self.model, DraftStateMixin):
raise Http404
return get_object_or_404(self.model, pk=unquote(str(self.pk)))

RevisionUnscheduleView:

def setup(self, request, pk, revision_id, *args, **kwargs):
super().setup(request, *args, **kwargs)
self.pk = pk
self.revision_id = revision_id
self.object = self.get_object()
self.revision = self.get_revision()
def get_object(self, queryset=None):
if not self.model or not issubclass(self.model, DraftStateMixin):
raise Http404
return get_object_or_404(self.model, pk=unquote(str(self.pk)))

#10746 and #11590 would be something to consider as well. Although, I'm wondering, if we're going to refactor PermissionCheckedMixin to make use of ModelPermissionTester when possible (to allow instance-specific application-level permission checks) when wagtail/rfcs#92 is implemented, how would that affect where we put db call...


def get_form_kwargs(self):
return {**super().get_form_kwargs(), "instance": self.get_object()}
def get_initial_form_instance(self):
return self.get_object()


class CopyView(CopyViewMixin, CreateView):
pass


class EditView(
Expand Down
28 changes: 18 additions & 10 deletions wagtail/snippets/tests/test_snippets.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
)
from wagtail.snippets.blocks import SnippetChooserBlock
from wagtail.snippets.models import SNIPPET_MODELS, register_snippet
from wagtail.snippets.views.snippets import CopyView
from wagtail.snippets.widgets import (
AdminSnippetChooser,
SnippetChooserAdapter,
Expand Down Expand Up @@ -974,20 +973,29 @@ def setUp(self):
StandardSnippet.snippet_viewset.get_url_name("copy"),
args=(self.snippet.pk,),
)
self.login()
self.user = self.login()

def test_without_permission(self):
self.user.is_superuser = False
self.user.save()
admin_permission = Permission.objects.get(
content_type__app_label="wagtailadmin", codename="access_admin"
)
self.user.user_permissions.add(admin_permission)

def test_simple(self):
response = self.client.get(self.url)
self.assertEqual(response.status_code, 302)
self.assertRedirects(response, reverse("wagtailadmin_home"))

def test_form_is_prefilled(self):
response = self.client.get(self.url)
self.assertEqual(response.status_code, 200)
self.assertTemplateUsed(response, "wagtailsnippets/snippets/create.html")

def test_form_prefilled(self):
request = RequestFactory().get(self.url)
view = CopyView()
view.model = StandardSnippet
view.setup(request, pk=self.snippet.pk)

self.assertEqual(view._get_initial_form_instance(), self.snippet)
# Ensure form is prefilled
soup = self.get_soup(response.content)
text_input = soup.select_one('input[name="text"]')
self.assertEqual(text_input.attrs.get("value"), "Test snippet")
Comment on lines +978 to +998
Copy link
Member Author

Choose a reason for hiding this comment

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

This copies the same test from ModelViewSet in e3d9233. If I had done this as well in that commit, the test would've failed and we could've caught this bug. But, oh well...



@override_settings(WAGTAIL_I18N_ENABLED=True)
Expand Down
14 changes: 3 additions & 11 deletions wagtail/snippets/views/snippets.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from django.core import checks
from django.core.exceptions import ImproperlyConfigured, PermissionDenied
from django.http import Http404
from django.shortcuts import get_object_or_404, redirect
from django.shortcuts import redirect
from django.urls import path, re_path, reverse, reverse_lazy
from django.utils.functional import cached_property
from django.utils.text import capfirst
Expand Down Expand Up @@ -262,16 +262,8 @@ def get_context_data(self, **kwargs):
return context


class CopyView(CreateView):
def get_object(self):
return get_object_or_404(self.model, pk=self.kwargs["pk"])

def _get_initial_form_instance(self):
instance = self.get_object()
# Set locale of the new instance
if self.locale:
instance.locale = self.locale
return instance
class CopyView(generic.CopyViewMixin, CreateView):
pass


class EditView(generic.CreateEditViewOptionalFeaturesMixin, generic.EditView):
Expand Down
Loading