Deleting form submission redirects to first listing page

[tbrlpld]

Issue Summary

When you delete a form submission (on a later pagination page) then after confirming, you are redirected to the first pagination page.

Steps to Reproduce

1. Load a fresh Wagtail Bakerydemo
2. Visit the "contact us" page.
3. Submit the form more than 22 times (so that there is more than 1 item on the second pagination page)
4. Visit the submission listing screen for the "contact us" page.
5. Navigate to the second pagination page.
6. Select a submission for deletion and confirm the deletion.
7. You are redirected. Notice that you now are on the first pagination page again.
8. Navigate to the second pagination page to confirm that there are still submissions listed there.

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 / no)

Technical details

• Python version: 3.11.4
• Django version: 4.2.13
• Wagtail version: 6.1.2
• Browser version: irrelevant

Working on this

Anyone can contribute to this. View our contributing guidelines, add a comment to the issue once you’re ready to start.

This could be a good first issue. Can probably be solved with a next parameter or similar on the delete confirm view and using the "current URL" as next on the listing page showing the delete action.

[zerolab]

This has to do with the fact that the submission delete view defines the success URL as https://github.com/wagtail/wagtail/blob/main/wagtail/contrib/forms/views.py#L112-L114

Also the submission list uses bulk actions for the deletion, and the form takes us directly to the deletion view without a next query parameter - https://github.com/wagtail/wagtail/blob/main/wagtail/contrib/forms/templates/wagtailforms/list_submissions.html#L4

[shentonfreude]

I'll take a whack at this, at sprint:Arnhem2024

[shentonfreude]

@khalidSafwany and I confirmed the bug, and that the fix by @bruecksen works, and his test passes in two separate dev environments so we think his fix is ready to merge.

[laymonage]

Thanks all for reporting, working on the fix, and testing it.

Cross-posting my thoughts from the Wagtail Slack:

The problem with the proposed solution is that, if you delete a number of submissions in a way that causes the pagination number to no longer exist, you'll be redirected to a 404 page instead of the listing.

For example, if you have 21 submissions, and you go to the second page and delete the only submission there, you'll get this:

It's a behaviour from Django's BaseListView (which we use), that raises a 404 if you provide a page parameter that's not within the range of the available objects:

https://github.com/django/django/blob/e2428292abaca4758a7508175d31667fe2dff57c/django/views/generic/list.py#L72-L79

While we technically can make it so that out-of-range pagination would just render the closest-available page, that sounds a bit like "magic".

Thinking in a more general approach, perhaps the need to "redirect to the page where you were on" wouldn't be that necessary if we had the ability to jump to a specific page (#12004)...

As the reporter @tbrlpld, do you have any suggestions on how this specific case should be handled?

[tbrlpld]

Thanks for investigating this thoroughly @laymonage. That is a valid case, but I think the behavior you describe (to fall back to the new last page) would be sensible. I was under the impression that that's actually how the Django Paginator works already 🤔 maybe I am mistaken about that.

[tbrlpld]

Ok just checked. Paginator.get_page() returns the last page for out-of-rage page numbers. I think it would make sense to follow that pattern.

https://docs.djangoproject.com/en/5.0/ref/paginator/#django.core.paginator.Paginator.get_page

From a user perspective, this also feels the most sensible, as the new page (and thus the items you see) are the closest to what you saw before.

[bruecksen]

@laymonage @tbrlpld thanks for you input. Should I add a fix then to return to the last page for out-of-range page numbers?

[tbrlpld]

That sounds good to me, but I would defer to @laymonage for the core team perspective.

[laymonage]

Ok just checked. Paginator.get_page() returns the last page for out-of-rage page numbers. I think it would make sense to follow that pattern.

Unfortunately this method isn't actually used by Django in their BaseListView. This means we need to override one of the pagination methods in the view, e.g. paginate_queryset, get_paginator, or the paginator_class, to make it so that it calls Paginator.get_page() instead of Paginator.page().

We previously had such an override in Wagtail, but it was removed in 8e106f4 since we thought there was no clear reason why we had to do it. A year later, this issue demonstrates why we had that override 😄

I've discussed this with @gasman and we're happy to reinstate the use of Paginator.get_page().

---

So, to proceed with this, we have a few options:

• Revert 8e106f4, which should fix the 404 issue.
  • Note: looking at Django's paginate_queryset() implementation, it adds additional logic to handle "last" as the parameter value. The override removed in that commit didn't have the logic to handle "last", but I don't think this is useful if we use get_page(), since we can just use -1.
• Alternatively, we can override the paginator_class to use a custom subclass of Paginator that overrides page() to call get_page() instead. Not sure if this is a good idea.

I think I'd be happy with reverting 8e106f4. It may not be straightforward to revert though, as we've made significant changes to the listing views code since then. Happy to provide additional help if needed!

[MaartenUreel]

Does this also apply to the fact that you are redirected to the first page of the listing after editing a page?

[bruecksen]

Does this also apply to the fact that you are redirected to the first page of the listing after editing a page?

Do you mean in the page tree? Or at a Viewset? (I don't know if that is different) In the bakery demo I could reproduce this in the Bread Ingredient list, editing one ingredient on the second page you end up at the first page. @laymonage any thoughts on this? Should this be handled in this issue as well?

If you do this in the page tree you also always end up at the first page but it is always sorted by updated desc, so you see the edited one at the top. I guess that is the intended behaviour there?

[bruecksen]

I think I'd be happy with reverting 8e106f4. It may not be straightforward to revert though, as we've made significant changes to the listing views code since then. Happy to provide additional help if needed!

@laymonage yes I guess I need some pointers in the right direction. I added the paginate_queryset() to the BaseListingView class. That gives us the correct handling of the out of range page. But is that the correct place for it?

If I then run the tests, lots of existing tests all over the place (TestImageIndexView.test_pagination, TestDocumentIndexView.test_pagination_out_of_range, ...) fail because of the new handling. Should these tests then be fixed or combined into one test at test_views_generic.py?

Thank you!

[laymonage]

@bruecksen Thanks! Yes, I think BaseListingView is the right place for it.

If I then run the tests, lots of existing tests all over the place (TestImageIndexView.test_pagination, TestDocumentIndexView.test_pagination_out_of_range, ...) fail because of the new handling. Should these tests then be fixed or combined into one test at test_views_generic.py?

The commit also included changes to those tests, so if we revert those changes as well, I think they should pass.