Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pagination Updated - Items per page option added #1625

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
87 changes: 87 additions & 0 deletions vulnerabilities/pagination_mixin.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
class PaginatedListViewMixin:
"""
A mixin that adds pagination functionality to ListView-based views.
Copy link
Member

Choose a reason for hiding this comment

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

I would call the file pagination.py instead

"""

paginate_by = 20
max_page_size = 100

PAGE_SIZE_CHOICES = [
{"value": 20, "label": "20 per page"},
{"value": 50, "label": "50 per page"},
{"value": 100, "label": "100 per page"},
]

def get_paginate_by(self, queryset=None):
Copy link
Author

Choose a reason for hiding this comment

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

This function handles page size from URL parameters. Sets default to 20 items, max of 100. Validates to ensure positive integers only. Returns default if invalid values provided.

"""
Get the number of items to paginate by from the request.
"""
try:
page_size = int(self.request.GET.get("page_size", self.paginate_by))
Copy link
Member

Choose a reason for hiding this comment

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

Is this the standard approach to get a request arg?

if page_size <= 0:
return self.paginate_by
return min(page_size, self.max_page_size)
except (ValueError, TypeError):
return self.paginate_by

def get_pagination_context(self, paginator, page_obj):
"""
Generate pagination-related context data, preserving filters.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Generate pagination-related context data, preserving filters.
Return a mapping of pagination-related context data, preserving filters.

"""
if not paginator or not page_obj:
return {}

current_page_size = self.get_paginate_by()
total_count = paginator.count

query_params = self.request.GET.copy()
query_params.pop("page", None)

base_query_string = query_params.urlencode()
Copy link
Member

Choose a reason for hiding this comment

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

Is this the right way to generate a URL query string? Is there a better way?

base_url = f"?{base_query_string}" if base_query_string else "?"

pages = []
if paginator.num_pages <= 5:
pages = [str(i) for i in range(1, paginator.num_pages + 1)]
else:
pages.append("1")

if page_obj.number > 3:
pages.append("...")

start_page = max(2, page_obj.number - 2)
end_page = min(paginator.num_pages - 1, page_obj.number + 2)
pages.extend(str(i) for i in range(start_page, end_page + 1))

if page_obj.number < paginator.num_pages - 2:
pages.append("...")

if str(paginator.num_pages) not in pages:
pages.append(str(paginator.num_pages))

return {
"current_page_size": current_page_size,
"page_size_choices": self.PAGE_SIZE_CHOICES,
"total_count": total_count,
"page_range": pages,
"search": self.request.GET.get("search", ""),
"base_url": base_url,
"previous_page_url": f"{base_url}&page={page_obj.previous_page_number}"
if page_obj.has_previous()
else None,
"next_page_url": f"{base_url}&page={page_obj.next_page_number}"
if page_obj.has_next()
else None,
}

def get_context_data(self, **kwargs):
"""
Add pagination context to the existing context data.
"""
context = super().get_context_data(**kwargs)
paginator = context.get("paginator")
page_obj = context.get("page_obj")
pagination_context = self.get_pagination_context(paginator, page_obj)
context.update(pagination_context)

return context
109 changes: 73 additions & 36 deletions vulnerabilities/templates/includes/pagination.html
Original file line number Diff line number Diff line change
@@ -1,39 +1,76 @@
<nav class="pagination is-centered is-small" aria-label="pagination">
{% if page_obj.has_previous %}
<a href="?page={{ page_obj.previous_page_number }}&search={{ search|urlencode }}" class="pagination-previous">Previous</a>
{% else %}
<a class="pagination-previous" disabled>Previous</a>
{% endif %}
<div class="pagination-controls mb-4">
<div class="is-flex is-justify-content-center mb-3">
<div class="select is-small {% if total_count < current_page_size %}is-disabled{% endif %}">
<select onchange="handlePageSizeChange(this.value)"
{% if total_count < current_page_size %}disabled="disabled"{% endif %}>
{% for choice in page_size_choices %}
<option value="{{ choice.value }}"
{% if choice.value == current_page_size %}selected{% endif %}>
{{ choice.label }}
</option>
{% endfor %}
</select>
</div>
</div>

Copy link
Contributor

Choose a reason for hiding this comment

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

All the complexity of those templates should likely be handled in Python.

{% if page_obj.has_next %}
<a href="?page={{ page_obj.next_page_number }}&search={{ search|urlencode }}" class="pagination-next">Next</a>
{% else %}
<a class="pagination-next" disabled>Next</a>
{% endif %}
{% if page_obj.paginator.num_pages > 1 %}
<nav class="pagination is-centered" role="navigation" aria-label="pagination">
{% if page_obj.has_previous %}
<a href="?page={{ page_obj.previous_page_number }}&search={{ search|urlencode }}&page_size={{ current_page_size }}"
class="pagination-previous">Previous</a>
{% else %}
<button class="pagination-previous" disabled>Previous</button>
{% endif %}

<ul class="pagination-list">
{% if page_obj.number != 1%}
<li>
<a href="?page=1&search={{ search|urlencode }}" class="pagination-link" aria-label="Goto page 1">1</a>
</li>
{% if page_obj.number > 2 %}
<li>
<span class="pagination-ellipsis">&hellip;</span>
</li>
{% endif %}
{% if page_obj.has_next %}
<a href="?page={{ page_obj.next_page_number }}&search={{ search|urlencode }}&page_size={{ current_page_size }}"
class="pagination-next">Next</a>
{% else %}
<button class="pagination-next" disabled>Next</button>
{% endif %}
<ul class="pagination-list">
{% for page_num in page_range %}
{% if page_num == '...' %}
<li><span class="pagination-ellipsis">&hellip;</span></li>
{% else %}
<li>
<a href="?page={{ page_num }}&search={{ search|urlencode }}&page_size={{ current_page_size }}"
class="pagination-link {% if page_num == page_obj.number %}is-current{% endif %}"
aria-label="Go to page {{ page_num }}"
{% if page_num == page_obj.number %}aria-current="page"{% endif %}>
{{ page_num }}
</a>
</li>
{% endif %}
{% endfor %}
</ul>
</nav>
{% endif %}
<li>
<a class="pagination-link is-current" aria-label="Page {{ page_obj.number }}" aria-current="page">{{ page_obj.number }}</a>
</li>
{% if page_obj.number != page_obj.paginator.num_pages %}
{% if page_obj.next_page_number != page_obj.paginator.num_pages %}
<li>
<span class="pagination-ellipsis">&hellip;</span>
</li>
{% endif %}
<li>
<a href="?page={{ page_obj.paginator.num_pages }}&search={{ search|urlencode }}" class="pagination-link" aria-label="Goto page {{ page_obj.paginator.num_pages }}">{{ page_obj.paginator.num_pages }}</a>
</li>
{% endif %}
</ul>
</nav>
</div>

<style>
.select.is-disabled {
opacity: 0.7;
cursor: not-allowed;
}
.select.is-disabled select {
cursor: not-allowed;
}
</style>

<script>
function handlePageSizeChange(value) {
const url = new URL(window.location.href);
const params = new URLSearchParams(url.search);
params.set('page_size', value);
params.delete('page');

const search = params.get('search');
if (search) {
params.set('search', search);
}

const newUrl = `${window.location.pathname}?${params.toString()}`;
window.location.href = newUrl;
}
</script>
Copy link
Author

Choose a reason for hiding this comment

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

This js function manages URL parameters when changing pagination size. It update the page size parameter while preserving search terms, and remove the current page parameter to reset pagination.

Copy link
Member

Choose a reason for hiding this comment

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

is there any risk of XSS there?

29 changes: 14 additions & 15 deletions vulnerabilities/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
from vulnerabilities.utils import get_severity_range
from vulnerablecode.settings import env

from .pagination_mixin import PaginatedListViewMixin
Copy link
Author

Choose a reason for hiding this comment

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

Import the Mixin and the PaginatedListViewMixin class.


PAGE_SIZE = 20


Expand All @@ -62,25 +64,21 @@ def get_purl_version_class(purl: models.Package):
return purl_version_class


class PackageSearch(ListView):
class PackageSearch(PaginatedListViewMixin, ListView):
Copy link
Author

Choose a reason for hiding this comment

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

Add the PaginatedListViewMixin in it and this would convert any ListView into paginated class.

"""
View for searching & displaying packages with pagination.
"""

model = models.Package
template_name = "packages.html"
ordering = ["type", "namespace", "name", "version"]
paginate_by = PAGE_SIZE

def get_context_data(self, **kwargs):
context = super().get_context_data(**kwargs)
request_query = self.request.GET
context["package_search_form"] = PackageSearchForm(request_query)
context["search"] = request_query.get("search")
context["package_search_form"] = PackageSearchForm(self.request.GET)
return context

def get_queryset(self, query=None):
"""
Return a Package queryset for the ``query``.
Make a best effort approach to find matching packages either based
on exact purl, partial purl or just name and namespace.
"""
query = query or self.request.GET.get("search") or ""
return (
self.model.objects.search(query)
Expand All @@ -90,17 +88,18 @@ def get_queryset(self, query=None):
)


class VulnerabilitySearch(ListView):
class VulnerabilitySearch(PaginatedListViewMixin, ListView):
"""
View for searching and displaying vulnerabilities with pagination.
"""

model = models.Vulnerability
template_name = "vulnerabilities.html"
ordering = ["vulnerability_id"]
paginate_by = PAGE_SIZE

def get_context_data(self, **kwargs):
context = super().get_context_data(**kwargs)
request_query = self.request.GET
context["vulnerability_search_form"] = VulnerabilitySearchForm(request_query)
context["search"] = request_query.get("search")
context["vulnerability_search_form"] = VulnerabilitySearchForm(self.request.GET)
return context

def get_queryset(self, query=None):
Expand Down