Remove unused snippets delete-multiple view #10099

laymonage · 2023-02-15T17:54:24Z

Fixes #10058.

Please check the following:

Do the tests still pass?¹
Does the code comply with the style guide?
- Run make lint from the Wagtail root.
For Python changes: Have you added tests to cover the new/fixed behaviour?
For front-end changes: Did you test on all of Wagtail’s supported environments?²
- Please list the exact browser and operating system versions you tested:
- Please list which assistive technologies ³ you tested:
For new features: Has the documentation been updated accordingly?

Please describe additional details for testing this change.

squash-labs · 2023-02-15T17:55:12Z

Manage this branch in Squash

Test this branch here: https://laymonageremove-unused-b5e1h.squash.io

laymonage · 2023-02-16T09:46:01Z

docs/reference/hooks.md

@@ -1166,7 +1166,7 @@ Called at the beginning of the create snippet view. Works in a similar way to `b

 ### `after_delete_snippet`

-Called when a Snippet is deleted. The callable passed into the hook will receive the model instance(s) as a queryset along with the request object. If the callable returns an `HttpResponse`, that response will be returned immediately to the user, and Wagtail will not proceed to call `redirect()` to the listing view.
+Called when a Snippet is deleted. The callable passed into the hook will receive the model instance(s) as a list along with the request object. If the callable returns an `HttpResponse`, that response will be returned immediately to the user, and Wagtail will not proceed to call `redirect()` to the listing view.


This has never been consistent since the hooks were introduced: if there's only one snippet to be deleted, the hook is called with a list, and a queryset otherwise.

The base bulk action view doesn't store a reference to the queryset, it's immediately evaluated and the items are checked for permissions, so we can't call the hook with the queryset:

wagtail/wagtail/admin/views/bulk_action/base_bulk_action.py

Lines 93 to 107 in 0187a14

def get_actionable_objects(self):

objects = []

items_with_no_access = []

object_ids = self.request.GET.getlist("id")

if "all" in object_ids:

object_ids = self.get_all_objects_in_listing_query(

self.request.GET.get("childOf")

)

for obj in self.get_queryset(self.model, object_ids):

if not self.check_perm(obj):

items_with_no_access.append(obj)

else:

objects.append(obj)

return objects, {"items_with_no_access": items_with_no_access}

I was thinking of storing the result in self.queryset somewhere, but weirdly the method is a classmethod:

wagtail/wagtail/admin/views/bulk_action/base_bulk_action.py

Lines 53 to 55 in 0187a14

@classmethod

def get_queryset(cls, model, object_ids):

return get_list_or_404(model, pk__in=object_ids)

And it uses get_list_or_404, which returns a list instead of a QuerySet.

I'm cautious to make any changes to the base bulk action view as it seems out of scope for this PR. Considering that this hook has been partially lost since 4.0, I think having this consistently be a list isn't so bad.

I considered adding a .. versionchanged here but I'm not sure what's the right way to document this, seeing that the hook has never been consistent, and the fact that it hasn't been accessible since 4.0.

laymonage · 2023-02-16T09:51:36Z

wagtail/admin/templates/wagtailadmin/generic/confirm_delete.html

@@ -6,5 +6,6 @@
    <form action="{{ view.get_delete_url }}" method="POST">
        {% csrf_token %}
        <input type="submit" value="{% trans 'Yes, delete' %}" class="button serious" />
+        <a href="{{ view.get_success_url }}" class="button button-secondary">{% trans "No, don't delete" %}</a>


I'm surprised this wasn't here before.

I added this so that the snippet delete template can reuse as much of the template as possible, so that I can fully reuse this in #10072

laymonage · 2023-02-16T09:52:14Z

wagtail/snippets/bulk_actions/delete.py

    display_name = _("Delete")
    action_type = "delete"
    aria_label = _("Delete selected snippets")
    template_name = "wagtailsnippets/bulk_actions/confirm_bulk_delete.html"
    action_priority = 30
    classes = {"serious"}

+    @cached_property
+    def _actionable_objects(self):


This is a bit tangled but I recommend reading the base bulk action view to see why I wrote it this way. In short, the base view's override points aren't as good as they could be.

laymonage · 2023-02-16T10:48:21Z

wagtail/snippets/bulk_actions/delete.py

+    def run_before_hook(self):
+        # The method returns a tuple of
+        # `(objects, {"items_with_no_access": items_with_no_access})`, hence [0]
+        objects = self.get_actionable_objects()[0]


Could also be self._actionable_objects[0], but that would bypass get_actionable_objects() if it has been overridden.

…on view

laymonage · 2023-03-01T09:50:50Z

FYI @gasman

The hooks situation in summary:

Snippets' DeleteView used to handle both single and multiple objects, and the hooks were called for both cases
In 4.0, we changed the multiple objects handling to use the bulk action view. The bulk action view has its own before/after hooks (which shares the same {before,after}_bulk_action hook name for all bulk actions, e.g. for pages, and for other non-delete views).
- The bulk action hooks are triggered just around the deletion code as opposed to the start/end of the view, so the before hook doesn't get called for GET requests.
- When these changes were made, the snippets' deletion hooks were not called in the bulk action view. This effectively means that the snippet hooks will no longer be triggered when deleting multiple snippets.
In this PR, I made it so that the original snippets hooks are called in the snippets' bulk action delete view, at the same points as the original hooks (start/end of the view). This restores the ability to trigger the original snippets hooks when deleting multiple snippets.
- There's a slight alteration here, as the hooks now receive a list of snippets instead of a queryset, due to how the bulk actions views were implemented. (See review comments above for details to this nuance.)
- As the code to handle multiple deletion has no longer been used in snippets' DeleteView since 4.0, I removed it.

gasman

All looks good to me!

gasman · 2023-03-14T14:29:13Z

Merged in 909e7fe

laymonage force-pushed the remove-unused branch from e8db6bc to a7728f9 Compare February 16, 2023 09:31

laymonage requested a review from gasman February 16, 2023 09:31

laymonage force-pushed the remove-unused branch from a7728f9 to 4820a3d Compare February 16, 2023 09:50

laymonage commented Feb 16, 2023

View reviewed changes

laymonage mentioned this pull request Feb 16, 2023

Remove unused snippets delete-multiple view #10058

Closed

laymonage commented Feb 16, 2023

View reviewed changes

laymonage mentioned this pull request Feb 16, 2023

Show on delete information in usage views to be linked from delete/unpublish confirmation views #10072

Merged

8 tasks

laymonage self-assigned this Feb 21, 2023

laymonage added type:Cleanup/Optimisation component:Snippets component:Bulk Actions labels Feb 21, 2023

laymonage added this to the 5.0 milestone Feb 21, 2023

laymonage added 4 commits February 21, 2023 15:37

Call {before,after}_delete_snippet hooks in snippets delete bulk acti…

608308d

…on view

Add "No, don't delete" link in generic delete confirmation template

ea3e5d3

Remove unused snippets delete-multiple view

c4d2bc2

Update docs to reflect snippet deletion hook changes

353646f

laymonage force-pushed the remove-unused branch from 4820a3d to 353646f Compare February 21, 2023 15:38

laymonage added the status:Needs Review label Feb 23, 2023

gasman approved these changes Mar 14, 2023

View reviewed changes

gasman added a commit that referenced this pull request Mar 14, 2023

Release note for #10099

71563ec

gasman closed this Mar 14, 2023

laymonage removed the status:Needs Review label Mar 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unused snippets delete-multiple view #10099

Remove unused snippets delete-multiple view #10099

laymonage commented Feb 15, 2023 •

edited

squash-labs bot commented Feb 15, 2023

laymonage Feb 16, 2023 •

edited

laymonage Feb 16, 2023

laymonage Feb 16, 2023

laymonage Feb 16, 2023

laymonage commented Mar 1, 2023 •

edited

gasman left a comment

gasman commented Mar 14, 2023

	def get_actionable_objects(self):
	objects = []
	items_with_no_access = []
	object_ids = self.request.GET.getlist("id")
	if "all" in object_ids:
	object_ids = self.get_all_objects_in_listing_query(
	self.request.GET.get("childOf")
	)

	for obj in self.get_queryset(self.model, object_ids):
	if not self.check_perm(obj):
	items_with_no_access.append(obj)
	else:
	objects.append(obj)
	return objects, {"items_with_no_access": items_with_no_access}

	@classmethod
	def get_queryset(cls, model, object_ids):
	return get_list_or_404(model, pk__in=object_ids)

Remove unused snippets delete-multiple view #10099

Remove unused snippets delete-multiple view #10099

Conversation

laymonage commented Feb 15, 2023 • edited

Footnotes

squash-labs bot commented Feb 15, 2023

Manage this branch in Squash

laymonage Feb 16, 2023 • edited

Choose a reason for hiding this comment

laymonage Feb 16, 2023

Choose a reason for hiding this comment

laymonage Feb 16, 2023

Choose a reason for hiding this comment

laymonage Feb 16, 2023

Choose a reason for hiding this comment

laymonage commented Mar 1, 2023 • edited

gasman left a comment

Choose a reason for hiding this comment

gasman commented Mar 14, 2023

laymonage commented Feb 15, 2023 •

edited

laymonage Feb 16, 2023 •

edited

laymonage commented Mar 1, 2023 •

edited