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
Conversation
Manage this branch in SquashTest this branch here: https://laymonagefix-copy-view-kyzvc.squash.io |
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") |
There was a problem hiding this comment.
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...
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])) | ||
) |
There was a problem hiding this comment.
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
:
wagtail/wagtail/admin/views/generic/models.py
Lines 737 to 741 in a09bba6
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()
:
wagtail/wagtail/admin/views/generic/models.py
Lines 951 to 958 in a09bba6
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
:
wagtail/wagtail/admin/views/generic/models.py
Lines 1054 to 1061 in a09bba6
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
:
wagtail/wagtail/admin/views/generic/models.py
Lines 1183 to 1191 in a09bba6
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
:
wagtail/wagtail/admin/views/generic/models.py
Lines 1286 to 1298 in a09bba6
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
:
wagtail/wagtail/admin/views/generic/models.py
Lines 1394 to 1404 in a09bba6
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sorting this @laymonage! It took a while to get my head around how this issue arose and what the "active" part of the fix is, so for reference: the refactoring of CreateView in ee57f6d didn't account for the fact that it was being subclassed in CopyView, and took the _get_initial_form_instance
method out of the control flow, meaning that the overridden version in CopyView was not being called. The fix to use pk_url_kwarg
in get_object
is not strictly related (I think), since the urlconf for CopyView registered by SnippetViewSet does use pk
- but it's a worthwhile fix to have in place anyhow.
@gasman Correct! Sorry, I should've added that context to make the review easier.
Yep, The use of I'll merge this now, unless you're currently merging it. Shall we cherry-pick this to |
Thanks! I'm merging now. I think it's worth pulling into 6.0.x too - even if we don't technically have to put out a bugfix release, I think it's a "good citizen" thing to do, so that we're not leaving 6.0.x in a broken state indefinitely. |
Fixes #11940.