Fixed #36175 -- Refactor admin pagination to make it reusable across different queryset. #19172

Antoliny0919 · 2025-02-13T11:03:09Z

Trac ticket number

Branch description

Provide a concise overview of the issue or rationale behind the proposed changes.
Refactored the pagination in the admin interface by separating the parts that were specifically designed for ChangeList, making the Pagination class reusable across different queryset.

Checklist

This PR targets the main branch.
The commit message is written in past tense, mentions the ticket number, and ends with a period.
I have checked the "Has patch" ticket flag in the Trac system.
I have added or updated relevant tests.
I have added or updated relevant docs, including release notes if applicable.
I have attached screenshots in both light and dark modes for any UI changes.

django/contrib/admin/options.py

nanuxbe

This looks like a good start.
My biggest concerns are about the missing deprecation if we are going with a changed API.
In order to validate that this is not breaking anything I would also duplicate the tests instead of changing them:

keep a copy of the old tests as they are + add a check that a deprecation warning is raised and a note to remove those after the deprecation cycle
add the changed version of the tests to validate the new API.

nanuxbe · 2025-02-18T12:13:50Z

django/contrib/admin/options.py

@@ -843,6 +843,14 @@ def get_changelist(self, request, **kwargs):

        return ChangeList

+    def get_pagination(self, request, **kwargs):


Given this returns a class, is there a reason not to use get_pagination_class instead of get_pagination?
See also comment below in the same area

nanuxbe · 2025-02-18T12:18:53Z

django/contrib/admin/options.py

-        page_obj = paginator.get_page(page_number)
-        page_range = paginator.get_elided_page_range(page_obj.number)
+        Pagination = self.get_pagination(request)
+        pagination = Pagination(


I feel like having both Pagination and pagination can be quite confusing, especially since we have get_pagination that returns the class and not the instance.

This would look better to me if we had:

pagination_class and get_pagination_class instead of Pagination and get_pagination

maybe also have a new get_pagination that would be in charge of calling get_pagination_class and instantiating the pagination class instead of directly calling the Pagination class.

This would be similar to a pattern we have in other places in Django, like in the FormView https://ccbv.co.uk/projects/Django/5.0/django.views.generic.edit/FormView/#get_form

nanuxbe · 2025-02-18T12:21:48Z

django/contrib/admin/templatetags/admin_list.py

@@ -35,36 +36,37 @@


 @register.simple_tag
-def paginator_number(cl, i):
+def paginator_number(p, i):


I'm aware you are replacing the cl abbreviation that is 2 letters with a similar-type abbreviation for pagination but p is probably too short. Could this be pagination instead of just p?

Agree, better to have descriptive variable names to make the code more readable and self-documenting.

nanuxbe · 2025-02-18T12:22:35Z

django/contrib/admin/templatetags/admin_list.py

            i,
        )


-def pagination(cl):
+def pagination(p):


Same as above, one-letter variable names outside of very specific use-cases isn't great

nanuxbe · 2025-02-18T12:23:06Z

django/contrib/admin/templatetags/admin_list.py

@@ -82,6 +84,23 @@ def pagination_tag(parser, token):
    )


+def changelist_pagination(p, cl):


one-letter variable-name here as well

nanuxbe · 2025-02-18T12:25:09Z

django/contrib/admin/utils.py

+        new_params = {}
+    if remove is None:
+        remove = []
+    p = params.copy()


Still not found of the one-letter variable names ;-)
p, k, r...

Even though I know you are just moving pre-existing code; if this piece of code is being touched, it's probably a good time to also make it more readable

nanuxbe · 2025-02-18T12:30:18Z

django/contrib/admin/views/main.py

    def get_results(self, request):
-        paginator = self.model_admin.get_paginator(
-            request, self.queryset, self.list_per_page
+        pagination = Pagination(


Calling Pagination directly seems less flexible than having a separate get_paginator method.

If feels like the recommended approach from above (get_pagination_class + get_pagination would be more flexible and extendable)

nanuxbe · 2025-02-18T12:36:25Z

django/contrib/admin/views/main.py

        self.show_full_result_count = self.model_admin.show_full_result_count
        # Admin actions are shown if there is at least one entry
        # or if entries are not counted because show_full_result_count is disabled
        self.show_admin_actions = not self.show_full_result_count or bool(
            full_result_count
        )
        self.full_result_count = full_result_count
-        self.result_list = result_list
-        self.can_show_all = can_show_all


The API of get_results is being changed here.
If this is the final approach, I don't see an issue with the change itself but we need to deprecate this properly:
ie:

deprecation warning

a workaround to populate the old properties while the deprecation is active

Antoliny0919 · 2025-02-19T00:34:58Z

This looks like a good start. My biggest concerns are about the missing deprecation if we are going with a changed API. In order to validate that this is not breaking anything I would also duplicate the tests instead of changing them:
* keep a copy of the old tests as they are + add a check that a deprecation warning is raised and a note to remove those after the deprecation cycle

* add the changed version of the tests to validate the new API.

Thank you, @nanuxbe! 😃 Since this is my first time handling such a issue, I may not have fully understood all the points you suggested.
I completely agree that refactoring should not break the existing logic. As you suggested, I will add deprecation warnings and write tests to validate the new API.
I also fully agree with all the other review points you provided.
(I feel that I may need to study more to better understand and build an improved structure.)

It might take me some time to complete this work, but I will make progress every day and ensure it’s not delayed too much. Once I believe the work is done, would it be okay to request review again from you?
I really appreciate your interest in this issue. Thank you!

nanuxbe · 2025-02-19T09:57:17Z

It might take me some time to complete this work, but I will make progress every day and ensure it’s not delayed too much. Once I believe the work is done, would it be okay to request review again from you? I really appreciate your interest in this issue. Thank you!

Yes, feel free to ping me when you think this is ready for another pass!
As said on trac, it's the least I can do ;-)

Antoliny0919 · 2025-02-21T07:34:48Z

Hello @nanuxbe !
you suggested in this ticket(24947) is about making the filter functionality used in the Admin available as a new API for Django users.

However, the ticket(36175) I am currently working on does not aim to fully expose Pagination as an API. My goal has been to make it customizable, extendable, and reusable within the Admin.

One thing I'm wondering about is whether the Pagination I am working on should also be made into a public API, as suggested in the ticket(24917)?
(e.g., django.core.pagination)

nanuxbe · 2025-02-24T07:31:28Z

@Antoliny0919 I am not sure what you are asking about. Paginator as it stands before this PR is already a public pagination tool, which can be used in CBVs and regular views (cf: https://docs.djangoproject.com/en/5.1/topics/pagination/#paginating-a-listview). Its API should stay stable (ie: you may add to it but should not remove/change).

AFAICT what you are working on here is a wrapper around Paginator for use in the admin so that several things there have the same pagination. I am not sure what benefit there would be to make that API available outside of the admin and make it available outside of the admin, can you elaborate on this?

Antoliny0919 · 2025-02-24T07:40:52Z

@nanuxbe Ah,,, I think I misunderstood. Your understanding is correct. I should focus on this task regardless of ticket(24947). sorry about that. 🥲

I haven't worked on the documentation yet, but if you don't mind, could you review the work I've done so far?
Would handling the deprecation as you previously suggested work like this?

nanuxbe · 2025-02-24T07:49:53Z

@Antoliny0919 Sure. My day today is going to get quite busy in about ten minutes though, so I'll probably only be able to give it a good review tomorrow or Wednesday

sarahboyce

Can you pull out the get_query_string refactor to a separate commit?

Instead of defining a new utility, we should use querystring
I think we should also consider using querystring directly in the template, and pass the arguments in the context

nanuxbe · 2025-02-27T16:24:57Z

django/contrib/admin/templatetags/admin_list.py

+        "replaced by an method in the Pagination class.",
+        RemovedInDjango70Warning,
+        stacklevel=2,
+    )


@Antoliny0919 the deprecation warnings look good to me

nanuxbe · 2025-03-19T13:17:16Z

@Antoliny0919 No pressure but I see there is no more activity here, is this ready for another review round?

Antoliny0919 · 2025-03-19T13:28:49Z

Yes @nanuxbe, there are still parts that need to be worked on after 35529 is merged, and the documentation has not been updated yet(This is the reason why I haven't marked the ticket as ready for review.). However, if you are willing to review it, I would appreciate it.

nanuxbe · 2025-03-26T17:05:38Z

django/contrib/admin/options.py

+        Return an instance of the `Pagination` class to be
+        used for paginating the queryset.
+        """
+        Pagination = self.get_pagination_class(request)


Since you are calling get_pagination_class without passing any kwargs, and get_pagination_class itself doesn't use the kwargs, I wonder if they should be removed from the method definition

Sorry for the late reply. I agree, it looks like kwargs aren’t necessary here.

nanuxbe · 2025-03-26T17:13:00Z

django/contrib/admin/options.py


        context = {
            **self.admin_site.each_context(request),
            "title": _("Change history: %s") % obj,
            "subtitle": None,
-            "action_list": page_obj,
-            "page_range": page_range,


These 3 params (page_range, page_var and pagination_required should probably be left in the context with a deprecation warning if accessed.
The use case here is that there are quite a few admin themes around and removing variables from a context will probably break a few of those

I agree. Suddenly removing them from the context could cause problems for people who were previously using those context variables.
However, I haven’t found a way to show a deprecation warning when those variables are accessed from the context.
Should I implement it myself? I’ll look into it a bit more for now!

nanuxbe · 2025-03-26T17:18:30Z

django/contrib/admin/paginations.py

+            return format_html("{} ", self.paginator.ELLIPSIS)
+        elif i == self.page_num:
+            return format_html('<span class="this-page">{}</span> ', i)
+        else:


My preference here would be to drop the else and un-indent the rest of the code. Ie: return early and have the main body of the function as the "default".
It's a just preference though

nanuxbe · 2025-03-26T17:23:41Z

django/contrib/admin/views/main.py

 from django.utils.http import urlencode
 from django.utils.timezone import make_aware
 from django.utils.translation import gettext

 # Changelist settings
-ALL_VAR = "all"


I wonder if those should be left there with a deprecation warning. I would expect one of the fellows would have an opinion about this

Antoliny0919 · 2025-03-30T00:40:34Z

Thank you for taking an interest in this work and for reviewing it @nanuxbe 🥰. I will reflect on the points you mentioned as soon as I have time.

cliff688 · 2025-05-30T10:20:30Z

django/contrib/admin/paginations.py

+
+    @property
+    def pagination_required(self):
+        return (not self.show_all or not self.can_show_all) and self.multi_page


Suggested change

return (not self.show_all or not self.can_show_all) and self.multi_page

self.multi_page and not (self.show_all and self.can_show_all)

I think reversing the order here is more readable

I agree. Thank you, cliff!

cliff688 · 2025-05-30T10:31:33Z

django/contrib/admin/paginations.py

+    def render_all_pages(self):
+        """
+        Generate the HTML for all page links in the pagination.
+        """


This name doesn't sound property-like. It sounds more like it does something, rather than representing something. Maybe something like:

Suggested change

def render_all_pages(self):

"""

Generate the HTML for all page links in the pagination.

"""

def all_rendered_pages(self):

"""HTML for all the pages in the pagination."""

Since it is provided as a property rather than a method, the name you suggested is more appropriate.
Hmm, but I also think it might be better if it were provided as a method rather than a property 🧐

cliff688 · 2025-05-30T10:41:51Z

django/contrib/admin/templatetags/admin_list.py

+        "paginator_number template tag has been "
+        "replaced by an method in the Pagination class.",


Suggested change

"paginator_number template tag has been "

"replaced by an method in the Pagination class.",

"paginator_number template tag has been replaced by an method in the Pagination "

"class.",

cliff688 · 2025-05-30T10:42:49Z

django/contrib/admin/views/main.py

 SEARCH_VAR = "q"
 ERROR_FLAG = "e"

+


Suggested change

cliff688 · 2025-05-30T10:43:20Z

django/contrib/admin/views/main.py

-        self.list_select_related = list_select_related
        self.list_per_page = list_per_page
        self.list_max_show_all = list_max_show_all
+        self.list_select_related = list_select_related


You can revert this 👍🏾

cliff688 · 2025-05-30T10:49:43Z

django/contrib/admin/paginations.py

I think pagination.py is a better name here

cliff688 · 2025-05-30T10:54:10Z

django/contrib/admin/views/main.py

+            "ChangeList().page_num attribute is deprecated. "
+            "use pagination.page_num instead.",


Suggested change

"ChangeList().page_num attribute is deprecated. "

"use pagination.page_num instead.",

"ChangeList().page_num attribute is deprecated. Use "

pagination.page_num instead.",

cliff688 · 2025-05-30T10:54:55Z

django/contrib/admin/views/main.py

+            "ChangeList().show_all attribute is deprecated. "
+            "use pagination.show_all instead.",


Ditto, as above. Capitalise second sentence

Also in the other warnings

cliff688 · 2025-05-30T10:58:02Z

docs/ref/contrib/admin/index.txt

-    all" admin change list page. The admin will display a "Show all" link on the
-    change list only if the total result count is less than or equal to this
-    setting. By default, this is set to ``200``.
+    all" admin page(change list, history). The admin will display a "Show all"


Suggested change

all" admin page(change list, history). The admin will display a "Show all"

all" admin page (change list, history). The admin will display a "Show all"

```suggestion

cliff688 · 2025-05-30T10:58:51Z

docs/ref/contrib/admin/index.txt


 .. attribute:: ModelAdmin.list_per_page

    Set ``list_per_page`` to control how many items appear on each paginated
-    admin change list page. By default, this is set to ``100``.
+    admin page(change list, history). By default, this is set to ``100``.


Suggested change

admin page(change list, history). By default, this is set to ``100``.

admin page (change list, history). By default, this is set to ``100``.

cliff688

Thanks @Antoliny0919 for your dedication to this work ⭐ !

I left some comments (from an incomplete review).

I think you should split your commit so that the deprecations are in a separate first commit and the fix in a different commit. That will make this a bit easier to review.

…different queryset.

Antoliny0919 · 2025-05-31T06:50:48Z

Thanks @Antoliny0919 for your dedication to this work ⭐ !

I left some comments (from an incomplete review).

I think you should split your commit so that the deprecations are in a separate first commit and the fix in a different commit. That will make this a bit easier to review.

Thank you for reviewing this work @cliff688 .
I had put this aside for a while, but now I’ll have to pick it up again!

Antoliny0919 · 2025-06-14T14:34:52Z

Could you please add the Selenium label? 😎

sarahboyce · 2025-08-06T14:33:05Z

@Antoliny0919 do you mind resolving the conflicts before this goes through another round of review? ⭐

Antoliny0919 · 2025-08-07T06:58:52Z

@Antoliny0919 do you mind resolving the conflicts before this goes through another round of review? ⭐

Nice!! I'm glad this work can be reviewed!
I'm preparing another ticket PR, so I'll submit that first and then try to get this done within this week as much as possible :)

Antoliny0919 commented Feb 13, 2025

View reviewed changes

django/contrib/admin/options.py Outdated Show resolved Hide resolved

Antoliny0919 force-pushed the ticket_36175 branch 3 times, most recently from 53a20fe to 69216c0 Compare February 16, 2025 10:01

nanuxbe suggested changes Feb 18, 2025

View reviewed changes

Antoliny0919 force-pushed the ticket_36175 branch from 69216c0 to 308139a Compare February 19, 2025 10:08

Antoliny0919 force-pushed the ticket_36175 branch 3 times, most recently from 9b69465 to 364bdd3 Compare February 23, 2025 11:57

Antoliny0919 requested a review from nanuxbe February 23, 2025 13:09

sarahboyce reviewed Feb 25, 2025

View reviewed changes

Antoliny0919 mentioned this pull request Feb 27, 2025

Refs #36175 -- Refactor admin to use querystring template tag. #19212

Open

nanuxbe reviewed Feb 27, 2025

View reviewed changes

Antoliny0919 force-pushed the ticket_36175 branch from 364bdd3 to d38d4de Compare February 28, 2025 09:10

nanuxbe reviewed Mar 26, 2025

View reviewed changes

Antoliny0919 force-pushed the ticket_36175 branch 2 times, most recently from c794504 to f772203 Compare May 28, 2025 12:08

cliff688 self-requested a review May 28, 2025 12:44

cliff688 reviewed May 30, 2025

View reviewed changes

django/contrib/admin/paginations.py Outdated

Copy link

Member

cliff688 May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think pagination.py is a better name here

Antoliny0919 reacted with thumbs up emoji

cliff688 reviewed May 30, 2025

View reviewed changes

Fixed #36175 -- Refactor admin pagination to make it reusable across …

50ba1d5

…different queryset.

Antoliny0919 force-pushed the ticket_36175 branch from f772203 to 50ba1d5 Compare May 31, 2025 06:22

Updated pagination tag, ChangeList footer css.

c1e4975

cliff688 added the selenium Apply to have Selenium tests run on a PR label Jun 14, 2025

Antoliny0919 mentioned this pull request Jun 25, 2025

Fixed #36366 -- Improved accessibility of pagination in the admin. #19448

Merged

6 tasks

sarahboyce added the screenshots 🖼️ label Aug 6, 2025

		@@ -843,6 +843,14 @@ def get_changelist(self, request, **kwargs):

		return ChangeList

		def get_pagination(self, request, **kwargs):

		@@ -82,6 +84,23 @@ def pagination_tag(parser, token):
		)


		def changelist_pagination(p, cl):

	return (not self.show_all or not self.can_show_all) and self.multi_page
	self.multi_page and not (self.show_all and self.can_show_all)

		"paginator_number template tag has been "
		"replaced by an method in the Pagination class.",

		"ChangeList().page_num attribute is deprecated. "
		"use pagination.page_num instead.",

		"ChangeList().show_all attribute is deprecated. "
		"use pagination.show_all instead.",

	all" admin page(change list, history). The admin will display a "Show all"
	all" admin page (change list, history). The admin will display a "Show all"
	```suggestion

	admin page(change list, history). By default, this is set to ``100``.
	admin page (change list, history). By default, this is set to ``100``.

Uh oh!

Fixed #36175 -- Refactor admin pagination to make it reusable across different queryset. #19172

Are you sure you want to change the base?

Fixed #36175 -- Refactor admin pagination to make it reusable across different queryset. #19172

Uh oh!

Conversation

Antoliny0919 commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Trac ticket number

Branch description

Checklist

Uh oh!

Uh oh!

nanuxbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Antoliny0919 commented Feb 19, 2025

Uh oh!

nanuxbe commented Feb 19, 2025

Uh oh!

Antoliny0919 commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nanuxbe commented Feb 24, 2025

Uh oh!

Antoliny0919 commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nanuxbe commented Feb 24, 2025

Uh oh!

sarahboyce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nanuxbe commented Mar 19, 2025

Uh oh!

Antoliny0919 commented Mar 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Antoliny0919 commented Mar 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Antoliny0919 commented Feb 13, 2025 •

edited

Loading

Antoliny0919 commented Feb 21, 2025 •

edited

Loading

Antoliny0919 commented Feb 24, 2025 •

edited

Loading