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.

Sign up for GitHub

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

Jump to bottom

Fix CopyView not prefilling the form data for snippets #11943

Closed
wants to merge 1 commit into from
Closed

Fix CopyView not prefilling the form data for snippets #11943

wants to merge 1 commit into from

Conversation

laymonage
Copy link
Member

Fixes #11940.

@squash-labs Squash Labs
Copy link

squash-labs bot commented May 10, 2024

Manage this branch in Squash

Test this branch here: https://laymonagefix-copy-view-kyzvc.squash.io

laymonage
laymonage commented May 10, 2024
View reviewed changes
wagtail/snippets/tests/test_snippets.py
Comment on lines +978 to +998
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")
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...

wagtail/admin/views/generic/models.py
Comment on lines +691 to +695
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]))
)
Copy link
Member Author

@laymonage laymonage May 10, 2024 •
edited

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...

@laymonage laymonage self-assigned this May 13, 2024
@laymonage laymonage requested a review from gasman May 13, 2024 08:39
@laymonage laymonage added this to the 6.1.1 milestone May 13, 2024
@laymonage laymonage assigned gasman and unassigned laymonage May 14, 2024
gasman
gasman approved these changes May 14, 2024
View reviewed changes
Copy link
Collaborator

@gasman gasman left a 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.

@laymonage
Copy link
Member Author

@gasman Correct! Sorry, I should've added that context to make the review easier.

The fix to use pk_url_kwarg in get_object is not strictly related (I think)

Yep, The use of pk_url_kwarg was done just for consistency since most of the other views use it as well, and we might want to unify how it's done in the future (as per my other comment).

I'll merge this now, unless you're currently merging it. Shall we cherry-pick this to stable/6.0.x as well (although a release isn't warranted)?

@gasman
Copy link
Collaborator

gasman commented May 14, 2024

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.

gasman added a commit that referenced this pull request May 14, 2024
@gasman
Release note for #11943 in 6.0.4
0c5b4e7
gasman added a commit that referenced this pull request May 14, 2024
@gasman
Release note for #11943 in 6.0.4
c97f033
gasman added a commit that referenced this pull request May 14, 2024
@gasman
Release note for #11943 in 6.1.1
75210ab
gasman added a commit that referenced this pull request May 14, 2024
@gasman
Release note for #11943 in 6.0.4
4b7210d
gasman added a commit that referenced this pull request May 14, 2024
@gasman
Release note for #11943 in 6.1.1
4f5ffa8
@gasman
Copy link
Collaborator

gasman commented May 14, 2024

Merged in 0c5b4e7 (stable/6.0.x) / 75210ab (stable/6.1.x) / 4f5ffa8 (main)

@gasman gasman closed this May 14, 2024
@laymonage laymonage mentioned this pull request May 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gasman gasman gasman approved these changes

Assignees

@gasman gasman

Labels
status:Needs Review type:Bug
Projects
Wagtail 6.2* release planning
Status: ✅ Done
Milestone
6.1.1
Development

Successfully merging this pull request may close these issues.

CopyView stopped to work for Snippets on Wagtail 6.1
2 participants
@laymonage @gasman