Repository: wagtail/wagtail
Pull request: Redirect to parent page when a child page is deleted from its listing view
Fixes #6527 

Do the tests still pass? Yes
    Does the code comply with the style guide? Yes
    For Python changes: Have you added tests to cover the new/fixed behaviour? No, I'm not really sure how to test it since it involves the frontend.


Problem statement:
Delete page in admin and get 404
### Issue Summary
When I delete the root page (a new project) I get a 404 in admin as after a delete Wagtail tries to go back to that page.



### Steps to Reproduce

1. start wagtail project
2. delete homepage that is made
3. get 404 page

I would expect to back to the three or see a screen that the page is deleted.

Any other relevant information. For example, why do you consider this a bug and what did you expect to happen instead?

* I have confirmed that this issue can be reproduced as described on a fresh Wagtail project: yes 


### Technical details

* Wagtail version: 2.11.1

Hints from issue discussion:
Thanks for the report @onno-timmerman! I can confirm that I'm seeing the same when I initiate the deletion from http://localhost:8000/admin/pages/3/ (but not http://localhost:8000/admin/pages/).
I think that this happens for all type of pages as long as we initiate the deletion from `/admin/pages/<page_id>/`. It's due to the fact that when we request a page like `/admin/pages/<page_id>/`, the request comes with a parameter `next` which equals `/admin/pages/<page_id/`. The [delete page view](https://github.com/wagtail/wagtail/blob/90a98f0cf5dd992960c7ea02b0a621664766bfb3/wagtail/admin/views/pages/delete.py#L13) uses this parameter [here](https://github.com/wagtail/wagtail/blob/90a98f0cf5dd992960c7ea02b0a621664766bfb3/wagtail/admin/views/pages/delete.py#L37-L39).

If we hover on the delete button from any page listing view on the admin interface, we can see an url like `/admin/pages/<page_id>/delete/?next=/admin/pages/<page_id/`. This means that wagtail will try to redirect us to the page we want to delete; which raises a 404 error.

As @gasman pointed, this error doesn't occur when we delete the page from its parent listing view. In that case the next parameter values to `/admin/pages/<parent_page_id/`.
From the parent page listing view, if we hover on the delete button of a child page,  we can see an url like `/admin/pages/<page_id>/delete/?next=/admin/pages/<parent_page_id/`, which means that we will be redirected to the parent page if we try to delete a child page from its parent page listing view.

So this avoid this error, I think we have to do a check [here](https://github.com/wagtail/wagtail/blob/90a98f0cf5dd992960c7ea02b0a621664766bfb3/wagtail/admin/wagtail_hooks.py#L211-L221) and update the `next_url` parameter.
We can:
-Either not add the new_url parameter to the url. To achieve it, we can do something like:
`if next_url:
          if next_url != reverse('wagtailadmin_explore', args=[page.id]):
                 url += '?' + urlencode({'next': next_url})` 
 In that case, the delete page view[(here)](https://github.com/wagtail/wagtail/blob/90a98f0cf5dd992960c7ea02b0a621664766bfb3/wagtail/admin/views/pages/delete.py#L39) redirects automatically to the parent page of the page we deleted.
-Or we can update the `new_url` parameter to redirect to another page.

What do you think about it?

Commit message: Rename button to delete_button

Patch to review:
diff --git a/wagtail/admin/tests/pages/test_delete_page.py b/wagtail/admin/tests/pages/test_delete_page.py
index fc624b809ccd..2a7f5430cdc5 100644
--- a/wagtail/admin/tests/pages/test_delete_page.py
+++ b/wagtail/admin/tests/pages/test_delete_page.py
@@ -5,13 +5,32 @@
 from django.http import HttpRequest, HttpResponse
 from django.test import TestCase
 from django.urls import reverse
+from django.utils.http import urlencode
 
+from wagtail.admin.wagtail_hooks import page_listing_more_buttons
 from wagtail.core.models import Page
 from wagtail.core.signals import page_unpublished
 from wagtail.tests.testapp.models import SimplePage, StandardChild, StandardIndex
 from wagtail.tests.utils import WagtailTestUtils
 
 
+class PagePerms:
+    def can_move(self):
+        return False
+
+    def can_copy(self):
+        return False
+
+    def can_delete(self):
+        return True
+
+    def can_unpublish(self):
+        return False
+
+    def can_view_revisions(self):
+        return False
+
+
 class TestPageDelete(TestCase, WagtailTestUtils):
     def setUp(self):
         # Find root page
@@ -202,3 +221,31 @@ def hook_func(request, page):
 
         # page should be deleted
         self.assertFalse(Page.objects.filter(id=self.child_page.id).exists())
+
+    def test_delete_button_next_url(self):
+        page_perms = PagePerms()
+        page = self.child_page
+        base_url = reverse('wagtailadmin_pages:delete', args=[page.id])
+
+        next_url = "a/random/url/"
+        full_url = base_url + '?' + urlencode({'next': next_url})
+
+        # page_listing_more_button generator yields only `Delete button`
+        delete_button = next(page_listing_more_buttons(
+            page,
+            page_perms,
+            is_parent=False,
+            next_url=next_url
+        ))
+
+        self.assertEqual(delete_button.url, full_url)
+
+        next_url = reverse('wagtailadmin_explore', args=[page.id])
+        delete_button = next(page_listing_more_buttons(
+            page,
+            page_perms,
+            is_parent=False,
+            next_url=next_url
+        ))
+
+        self.assertEqual(delete_button.url, base_url)
diff --git a/wagtail/admin/wagtail_hooks.py b/wagtail/admin/wagtail_hooks.py
index 479c6185e0ad..9899168fe9d2 100644
--- a/wagtail/admin/wagtail_hooks.py
+++ b/wagtail/admin/wagtail_hooks.py
@@ -210,7 +210,9 @@ def page_listing_more_buttons(page, page_perms, is_parent=False, next_url=None):
         )
     if page_perms.can_delete():
         url = reverse('wagtailadmin_pages:delete', args=[page.id])
-        if next_url:
+
+        # After deleting the page, it is impossible to redirect to it.
+        if next_url and next_url != reverse('wagtailadmin_explore', args=[page.id]):
             url += '?' + urlencode({'next': next_url})
 
         yield Button(
@@ -230,7 +232,6 @@ def page_listing_more_buttons(page, page_perms, is_parent=False, next_url=None):
             attrs={'title': _("Unpublish page '%(title)s'") % {'title': page.get_admin_display_title()}},
             priority=40
         )
-
     if page_perms.can_view_revisions():
         yield Button(
             _('History'),


Code review comments:
diff_hunk: @@ -210,7 +210,9 @@ def page_listing_more_buttons(page, page_perms, is_parent=False, next_url=None):
         )
     if page_perms.can_delete():
         url = reverse('wagtailadmin_pages:delete', args=[page.id])
-        if next_url:
+
+        # After deleting the page, it is impossible to redirect to it.
+        if next_url and next_url != reverse('wagtailadmin_explore', args=[page.id]):
path: wagtail/admin/wagtail_hooks.py
text: @user1:
Just an opinion, it feels like this bit of logic should be in its own block for added clarity to the comment:

```python
# After deleting the page, it is impossible to redirect to it.
if next_url != reverse('wagtailadmin_explore', args=[page.id]):
    next_url = None

if next_url:
```

diff_hunk: @@ -202,3 +221,31 @@ def hook_func(request, page):
 
         # page should be deleted
         self.assertFalse(Page.objects.filter(id=self.child_page.id).exists())
+
+    def test_delete_button_next_url(self):
+        page_perms = PagePerms()
+        page = self.child_page
+        base_url = reverse('wagtailadmin_pages:delete', args=[page.id])
+
+        next_url = "a/random/url/"
+        full_url = base_url + '?' + urlencode({'next': next_url})
+
+        # page_listing_more_button generator yields only `Delete button`
+        delete_button = next(page_listing_more_buttons(
path: wagtail/admin/tests/pages/test_delete_page.py
text: @user1:
We shouldn't be testing ``page_listing_more_buttons`` here. We should either test this behaviour at the view level (by asserting on the HTML output or template context), or we should move this into a separate unit test suite for `page_listing_more_buttons`

@author:
Should I move the tests into wagtail/admin/tests/test_button_hooks.py ?

@user1:
Yep, that sounds good to me, thanks!

@author:
It's done!

@user1:
awesome, thanks!