From e9f86cd602209faf387c733eddba8876e31a3161 Mon Sep 17 00:00:00 2001 From: Min RK Date: Wed, 28 Oct 2020 16:45:19 +0100 Subject: [PATCH] make pagination configurable add some unittests for pagination reorganize pagination a bit to make it easier to configure --- jupyterhub/app.py | 3 +- jupyterhub/handlers/pages.py | 8 +- jupyterhub/pagination.py | 171 +++++++++++++++++----------- jupyterhub/tests/test_pagination.py | 45 ++++++++ 4 files changed, 155 insertions(+), 72 deletions(-) create mode 100644 jupyterhub/tests/test_pagination.py diff --git a/jupyterhub/app.py b/jupyterhub/app.py index 9b11fef0..407f6bdb 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -77,6 +77,7 @@ from .user import UserDict from .oauth.provider import make_provider from ._data import DATA_FILES_PATH from .log import CoroutineLogFormatter, log_request +from .pagination import Pagination from .proxy import Proxy, ConfigurableHTTPProxy from .traitlets import URLPrefix, Command, EntryPointType, Callable from .utils import ( @@ -279,7 +280,7 @@ class JupyterHub(Application): @default('classes') def _load_classes(self): - classes = [Spawner, Authenticator, CryptKeeper] + classes = [Spawner, Authenticator, CryptKeeper, Pagination] for name, trait in self.traits(config=True).items(): # load entry point groups into configurable class list # so that they show up in config files, etc. diff --git a/jupyterhub/handlers/pages.py b/jupyterhub/handlers/pages.py index 4298dace..e336bcc7 100644 --- a/jupyterhub/handlers/pages.py +++ b/jupyterhub/handlers/pages.py @@ -453,7 +453,7 @@ class AdminHandler(BaseHandler): @web.authenticated @admin_only async def get(self): - page, per_page, offset = Pagination.get_page_args(self) + page, per_page, offset = Pagination(config=self.config).get_page_args(self) available = {'name', 'admin', 'running', 'last_activity'} default_sort = ['admin', 'name'] @@ -511,7 +511,11 @@ class AdminHandler(BaseHandler): total = self.db.query(orm.User.id).count() pagination = Pagination( - url=self.request.uri, total=total, page=page, per_page=per_page, + url=self.request.uri, + total=total, + page=page, + per_page=per_page, + config=self.config, ) auth_state = await self.current_user.get_auth_state() diff --git a/jupyterhub/pagination.py b/jupyterhub/pagination.py index 7f2450ca..8d4b912d 100644 --- a/jupyterhub/pagination.py +++ b/jupyterhub/pagination.py @@ -1,69 +1,94 @@ """Basic class to manage pagination utils.""" # Copyright (c) Jupyter Development Team. # Distributed under the terms of the Modified BSD License. +from traitlets import Bool +from traitlets import default +from traitlets import Integer +from traitlets import observe +from traitlets import Unicode +from traitlets import validate +from traitlets.config import Configurable -class Pagination: +class Pagination(Configurable): - _page_name = 'page' - _per_page_name = 'per_page' - _default_page = 1 - _default_per_page = 100 - _max_per_page = 250 + # configurable options + default_per_page = Integer( + 100, + config=True, + help="Default number of entries per page for paginated results.", + ) - def __init__(self, *args, **kwargs): - """Potential parameters. - **url**: URL in request - **page**: current page in use - **per_page**: number of records to display in the page. By default 100 - **total**: total records considered while paginating - """ - self.page = kwargs.get(self._page_name, 1) + max_per_page = Integer( + 250, + config=True, + help="Maximum number of entries per page for paginated results.", + ) - if self.per_page > self._max_per_page: - self.per_page = self._max_per_page + # state variables + url = Unicode("") + page = Integer(1) + per_page = Integer(1, min=1) - self.total = int(kwargs.get('total', 0)) - self.url = kwargs.get('url') or self.get_url() - self.init_values() + @default("per_page") + def _default_per_page(self): + return self.default_per_page - def init_values(self): - self._cached = {} - self.skip = (self.page - 1) * self.per_page - pages = divmod(self.total, self.per_page) - self.total_pages = pages[0] + 1 if pages[1] else pages[0] + @validate("per_page") + def _limit_per_page(self, proposal): + if self.max_per_page and proposal.value > self.max_per_page: + return self.max_per_page + if proposal.value <= 1: + return 1 + return proposal.value - self.has_prev = self.page > 1 - self.has_next = self.page < self.total_pages + @observe("max_per_page") + def _apply_max(self, change): + if change.new: + self.per_page = min(change.new, self.per_page) + + total = Integer(0) + + total_pages = Integer(0) + + @default("total_pages") + def _calculate_total_pages(self): + total_pages = self.total // self.per_page + if self.total % self.per_page: + # there's a remainder, add 1 + total_pages += 1 + return total_pages + + @observe("per_page", "total") + def _update_total_pages(self, change): + """Update total_pages when per_page or total is changed""" + self.total_pages = self._calculate_total_pages() + + separator = Unicode("...") - @classmethod def get_page_args(self, handler): """ This method gets the arguments used in the webpage to configurate the pagination In case of no arguments, it uses the default values from this class - It returns: - - self.page: The page requested for paginating or the default value (1) - - self.per_page: The number of items to return in this page. By default 100 and no more than 250 - - self.per_page * (self.page - 1): The offset to consider when managing pagination via the ORM + Returns: + - page: The page requested for paginating or the default value (1) + - per_page: The number of items to return in this page. No more than max_per_page + - offset: The offset to consider when managing pagination via the ORM """ - self.page = handler.get_argument(self._page_name, self._default_page) - self.per_page = handler.get_argument( - self._per_page_name, self._default_per_page - ) + page = handler.get_argument("page", 1) + per_page = handler.get_argument("per_page", self.default_per_page) try: - self.per_page = int(self.per_page) - if self.per_page > self._max_per_page: - self.per_page = self._max_per_page - except: + self.per_page = int(per_page) + except Exception: self.per_page = self._default_per_page try: - self.page = int(self.page) + self.page = int(page) if self.page < 1: - self.page = self._default_page + self.page = 1 except: - self.page = self._default_page + self.page = 1 return self.page, self.per_page, self.per_page * (self.page - 1) @@ -81,48 +106,54 @@ class Pagination: return {'total': self.total, 'start': start, 'end': end} def calculate_pages_window(self): - """Calculates the set of pages to render later in links() method. - It returns the list of pages to render via links for the pagination + """Calculates the set of pages to render later in links() method. + It returns the list of pages to render via links for the pagination By default, as we've observed in other applications, we're going to render only a finite and predefined number of pages, avoiding visual fatigue related to a long list of pages. By default, we render 7 pages plus some inactive links with the characters '...' to point out that there are other pages that aren't explicitly rendered. - The primary way of work is to provide current webpage and 5 next pages, the last 2 ones + The primary way of work is to provide current webpage and 5 next pages, the last 2 ones (in case the current page + 5 does not overflow the total lenght of pages) and the first one for reference. """ - self.separator_character = '...' - default_pages_to_render = 7 - after_page = 5 - before_end = 2 + before_page = 2 + after_page = 2 + window_size = before_page + after_page + 1 - # Add 1 to self.total_pages since our default page is 1 and not 0 - total_pages = self.total_pages + 1 + # Add 1 to total_pages since our starting page is 1 and not 0 + last_page = self.total_pages pages = [] - if total_pages > default_pages_to_render: - if self.page > 1: - pages.extend([1, '...']) + # will default window + start, end fit without truncation? + if self.total_pages > window_size + 2: + if self.page - before_page > 1: + # before_page will not reach page 1 + pages.append(1) + if self.page - before_page > 2: + # before_page will not reach page 2, need separator + pages.append(self.separator) - if total_pages < self.page + after_page: - pages.extend(list(range(self.page, total_pages))) + pages.extend(range(max(1, self.page - before_page), self.page)) + # we now have up to but not including self.page + + if self.page + after_page + 1 >= last_page: + # after_page gets us to the end + pages.extend(range(self.page, last_page + 1)) else: - if total_pages >= self.page + after_page + before_end: - pages.extend(list(range(self.page, self.page + after_page))) - pages.append('...') - pages.extend(list(range(total_pages - before_end, total_pages))) - else: - pages.extend(list(range(self.page, self.page + after_page))) - if self.page + after_page < total_pages: - # show only last page when the after_page window left space to show it - pages.append('...') - pages.extend(list(range(total_pages - 1, total_pages))) + # add full after_page entries + pages.extend(range(self.page, self.page + after_page + 1)) + # add separator *if* this doesn't get to last page - 1 + if self.page + after_page < last_page - 1: + pages.append(self.separator) + pages.append(last_page) return pages else: - return list(range(1, total_pages)) + # everything will fit, nothing to think about + # always return at least one page + return list(range(1, last_page + 1)) or [1] @property def links(self): @@ -155,9 +186,11 @@ class Pagination: page=page ) ) - elif page == self.separator_character: + elif page == self.separator: links.append( - '
    • ' + '
    • '.format( + separator=self.separator + ) ) else: links.append( diff --git a/jupyterhub/tests/test_pagination.py b/jupyterhub/tests/test_pagination.py new file mode 100644 index 00000000..b9833eae --- /dev/null +++ b/jupyterhub/tests/test_pagination.py @@ -0,0 +1,45 @@ +"""tests for pagination""" +from pytest import mark +from pytest import raises +from traitlets.config import Config + +from jupyterhub.pagination import Pagination + + +def test_per_page_bounds(): + cfg = Config() + cfg.Pagination.max_per_page = 10 + p = Pagination(config=cfg, per_page=20, total=100) + assert p.per_page == 10 + with raises(Exception): + p.per_page = 0 + + +@mark.parametrize( + "page, per_page, total, expected", + [ + (1, 10, 99, [1, 2, 3, "...", 10]), + (2, 10, 99, [1, 2, 3, 4, "...", 10]), + (3, 10, 99, [1, 2, 3, 4, 5, "...", 10]), + (4, 10, 99, [1, 2, 3, 4, 5, 6, "...", 10]), + (5, 10, 99, [1, "...", 3, 4, 5, 6, 7, "...", 10]), + (6, 10, 99, [1, "...", 4, 5, 6, 7, 8, "...", 10]), + (7, 10, 99, [1, "...", 5, 6, 7, 8, 9, 10]), + (8, 10, 99, [1, "...", 6, 7, 8, 9, 10]), + (9, 10, 99, [1, "...", 7, 8, 9, 10]), + (1, 20, 99, [1, 2, 3, 4, 5]), + (1, 10, 0, [1]), + (1, 10, 1, [1]), + (1, 10, 10, [1]), + (1, 10, 11, [1, 2]), + (1, 10, 50, [1, 2, 3, 4, 5]), + (1, 10, 60, [1, 2, 3, 4, 5, 6]), + (1, 10, 70, [1, 2, 3, 4, 5, 6, 7]), + (1, 10, 80, [1, 2, 3, "...", 8]), + ], +) +def test_window(page, per_page, total, expected): + cfg = Config() + cfg.Pagination + pagination = Pagination(page=page, per_page=per_page, total=total) + assert pagination.calculate_pages_window() == expected