-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Description
The feature to delete multiple snippets was added in #4610 (see also #3802). It was implemented using a combination of custom JavaScript and refactoring the delete view to handle multiple objects, which is then hooked to another url pattern. Later, the view was refactored to be a subclass of the generic DeleteView
in #8346, retaining the multiple objects logic.
As of #8574, deleting multiple snippets is implemented using the bulk actions framework. The delete bulk action uses its own view, and the multiple delete mechanism on the listing view was rerouted to use the bulk actions feature. This makes the delete multiple logic on the snippets' DeleteView
redundant, and in fact there's no way to access the view other than through the URL directly:
wagtail/wagtail/snippets/views/snippets.py
Lines 1109 to 1110 in f9872fa
path("multiple/delete/", self.delete_view, name="delete-multiple"), | |
path("delete/<str:pk>/", self.delete_view, name="delete"), |
I'd suggest that we remove the multiple objects handling logic from the view, so it can reuse the code from the generic delete view instead:
wagtail/wagtail/snippets/views/snippets.py
Lines 387 to 401 in f9872fa
def get_object(self, queryset=None): | |
# DeleteView requires either a pk kwarg or a positional arg, but we use | |
# an `id` query param for multiple objects. We need to explicitly override | |
# this so that we don't have to override `post()`. | |
return None | |
def get_objects(self): | |
# Replaces get_object to allow returning multiple objects instead of just one | |
if self.pk: | |
return [get_object_or_404(self.model, pk=unquote(self.pk))] | |
ids = self.request.GET.getlist("id") | |
objects = self.model.objects.filter(pk__in=ids) | |
return objects |
In addition, the delete-multiple
URL should also be removed.
Reducing this code duplication would allow us to use less code when implementing #10008.
Given that this code has been left unused since 4.0, I think it's fair to remove this without a deprecation process in 5.0.
Additionally, looks like the {before,after}_delete_snippet
hook wasn't included in #8574. This means that realistically the hook can only be triggered when deleting a single snippet. I'm not sure what approach we should do here:
- Call the
{before,after}_delete_snippet
hooks inside the delete bulk action view. - However, bulk actions have their own hooks, so maybe we could just instruct people to use that instead?
- Although these hooks called in
form_valid
instead of the beginning ofdispatch()
, so thebefore_bulk_action
hook won't get called on aGET
request...
- Although these hooks called in
I also noticed that the docs that the hooks accept a queryset of instances, when in reality it's not always the case: when there's only a single snippet, the hooks are passed a list of the single instance, not a queryset. This has been the case since the hooks were introduced in #6269 though, so probably not a big deal.